Firefox adblock option "Obj-Tabs" causes flash animations disappear

Created attachment 196531
Flashblock.xml see line 190

Created attachment 197053
flash file for testcase

Created attachment 197054
testcase

I get this js error too with this testcase (also using Flash8).
Those js errors seem to be here in the source code:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsJSNPRuntime.cpp#870

Martijn, are you seeing this in a debug build? If so, could you break at that
line and check why we're getting into that case? Is the NPObject null there?

Ryan, do you know what could be happening here?

So Martijn stepped through this in a debugger a tad. We're hitting the
ThrowJSException call at
http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsJSNPRuntime.cpp#1130
with the following stack:

#0 NPObjWrapper_NewResolve(JSContext*, JSObject*, long, unsigned, JSObject**)
    (cx=0xd813d10, obj=0xd5741f8, id=78235236, flags=1, objp=0x22efc8)
    at c:/mozilla/mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp:1131
#1 0x6f2710d4 in js_LookupPropertyWithFlags (cx=0xd813d10, obj=0xd5741f8,
    id=78250592, flags=1, objp=0x22f09c, propp=0x22f098)
    at c:/mozilla/mozilla/js/src/jsobj.c:2594
...
#8 0x05206d6d in nsJSContext::CallEventHandler(JSObject*, JSObject*, unsigned,
long*, long*) (this=0xd813c40, aTarget=0xd5560b0, aHandler=0xd574150,
    argc=1, argv=0xd8580c0, rval=0x22fa54)
    at c:/mozilla/mozilla/dom/src/base/nsJSEnvironment.cpp:1427
#9 0x05223e6f in nsGlobalWindow::RunTimeout(nsTimeout*) (this=0xd8e59c8,
    aTimeout=0xeea5428)
    at c:/mozilla/mozilla/dom/src/base/nsGlobalWindow.cpp:6185

At this point, npobj is null. Attempting to get the JSClass of |obj| complains
about invalid memory reads and doesn't return anything (in Martijn's debugger).

Now in frame 1 "id & 3 == 0". So it's an atom. And we have:

> print *(JSAtom*)78250592
$7 = {entry = {next = 0x0, keyHash = 1198838323, key = 0x4a9c664,
    value = 0x0}, flags = 2, number = 4705}
(gdb) print 0x4a9c664
$5 = 78235236

Which matches the value of |id| in the resolve hook. That's great, but:

(gdb) print 78235236 & 7
$6 = 4

So it's a string.

> print *(JSString*)(78235236 & ~7)
$8 = {length = 226573584, chars = 0x20}

Which is not a happy string.

So my best guess is that this string is dead. So is |obj|, most likely. So
somewhere GC happened and things were not protected from it...

Brendan, Blake, want to check this out? There's no Flash 8 for Linux yet, so
I'd need to get a Windows env running to look....

aren't this a blocker for 1.5? flash 8 objects are potential useless because of this.

Adding Michelle Sintov of Macromedia to the CC list in the hopes that she can shed some light on this bug.

If you think this should be a blocker, please set the blocking request flag and describe in detail the impact of this bug so that it can be evaluted.

From the Flash Player perspective...

In Flash Player 7, we did not support NPRuntime. We supported the older-school XPConnect JavaScript interface. In FP8 we moved to support NPRuntime.

With a recent Deer Park, Flash Player 8r22 and the testcase attachment (note the other test case links were Forbidden to me for some reason), I see the following:

NPP_Initialize and NPP_New are called.

NPP_GetValue is called for NPPVpluginScriptableNPObject, which causes us to call NPN_CreateObject, which causes Deer Park to call NPObject::NPAllocate() on us. After this, we call NPN_RetainObject().

NPP_Destroy is called, and NP_Deallocate is called on the same NPObject we created above.

NPP_Shutdown is called.

If it helps, you can likely test this same blocker on the NPRuntime sample plug-in and get the same results, as we copied that sample as closely as possible, and we've reviewed our NPRuntime code with Brendan. But, we could potentially have a bug in Flash, of course.

Brendan, jst, any idea what's going on here?

Created attachment 201809
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

What's happening here is that flash block touches the flash plugin element on page load only to remove it, when it does that we end up getting the plugin's scriptable object and sticking it on the plugin element's prototype chain. Once the plugin element is removed the plugin instance is destroyed and the plugin's scriptable object is invalidated (its private data is set to null), then the flash block code (or the testcase code, not sure, and it doesn't matter either) tries to use the plugin element, and when it does that we end up resolving properties on the plugin element's JS object, which ends up going down the prototype chain to the dead plugin's scriptable object. And there we throw the error.

I don't see any problems here with object's being GC'd or anything like that, it all looks fine to me in the debugger, and this all makes sense.

The fix makes the plugin scriptability code take the plugin scriptability object out of the plugin element's prototype chain when the plugin is destroyed.

Comment on attachment 201809
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

For what it's worth, r=bzbarsky

Comment on attachment 201809
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

r=mrbkap

Comment on attachment 201809
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

sr=me.

/be

Comment on attachment 201809
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

Requesting approval for various branches...

Fixed on the trunk.

Confirmed fixed on trunk with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051104 Firefox/1.6a1. Thanks for the fast work.

I tested this on Mac using today's trunk build. Installed Flashblock 1.3.3. I can't get Martijn's test case in Comment 3 to work. I am going to try to reproduce on another machine.

I retested this on Mac and it looks good using today's build. I am able to pass the reporter's testcase as well as the one in comment 3. Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051104 Firefox/1.6a1. I also took a quick look at the Windows build and it works fine too. Marking verified.

Fix landed on the 1.8 branch.

flag pixie dust

I agree with Henrik: will this be fixed in 1.5? From reading the comments, I don't get that impression.

(In reply to comment #22)
> I agree with Henrik: will this be fixed in 1.5? From reading the comments, I
> don't get that impression.
Yes, it will be fixed in 1.5. "fixed1.8" in the Keywords means it's fixed on branch.

(In reply to comment #23)
> (In reply to comment #22)
> > I agree with Henrik: will this be fixed in 1.5? From reading the comments, I
> > don't get that impression.
> Yes, it will be fixed in 1.5. "fixed1.8" in the Keywords means it's fixed on
> branch.
>

getting:
Error: Error calling method on NPObject!
is that another bug or a part of the same?

see bug https://bugzilla.mozilla.org/show_bug.cgi?id=308814

(In reply to comment #24)
> getting:
> Error: Error calling method on NPObject!
> is that another bug or a part of the same?
>
> see bug https://bugzilla.mozilla.org/show_bug.cgi?id=308814

Bug 308814 is a different bug (although probably related), this bug is fixed.

I think this caused bug 316025.

I'm still seeing this on Firefox 1.5RC3. Did somebody forget to land this on the Gecko 1.8 branch?

No, this was checked in on the branch. Check the CVS logs if you want.

A more important question. Did you see the problem in RC2? Or just RC3? One of the two changes from RC2 to RC3 was in this code (see bug 316025).

Regression from bug 316025 ?!?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107 Firefox/1.5 - RC2 OK
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051110 Firefox/1.5 - OK
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 RC3 - NOK "Bad NPObject..."
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051112 Firefox/1.5 - NOK "Bad NPObject..."

Um... Would the fact that we're passing in nsISupports instead of nsIDOMElement matter at all? I don't see how it would, but...

jst? Thoughts?

Created attachment 203609
This should fix it

Makes sure to look in the right scope... That doesn't matter for WrapNative, but does for GetWrappedNativeOfNativeObj, unfortunately.

That patch fixes the testcase for me, day old Windows branch build w/Flash 8. No JS Console error, reappears as described.

Comment on attachment 203609
This should fix it

r+sr=jst

> jst: approval1.8.0.1?

Does this mean that the fix won't make Firefox 1.5 final?

Probably not, no. 1.5 final is in code freeze at this point.

Fixed on trunk.

Comment on attachment 203609
This should fix it

Please land on 1.8 and 1.8.1 branches.

Could someone please land that last patch on branch? If not, I'll do it in early January...

1.8 Branch:
mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp; new revision: 1.7.2.4;
1.8.0 Branch:
mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp; new revision: 1.7.2.3.2.1;

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1, flash works as expected without errors with testcase in comment #3.

Is there any possibility of landing this on Mozilla 1.7.13 seeing that there isn't an *official* upgrade path supported by MoFo/MoCo?

You'd have to address that to drivers...

I have written a scriptable plugin (with the new NSAPI). I use Firefox 1.5.0.1 on MacOS 10.4.
If I set style.display="none" on the plugin, I still get errors "Bad NPObject as private data!" when I try to access scriptable objects created by the plugin from javascript:
  var plugin = document.getElementById("myplugin");
  var obj = plugin.getAnObject();
  var info1 = obj.getInfo(); //OK
  plugin.style.display = "none";
  var info2 = obj.getInfo(); // error "Bad NPObject as private data"
Is it the same bug ?

> If I set style.display="none" on the plugin

That's bug 90268.

Regression or new bug?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060426 Minefield/3.0a1

Error: Error calling method on NPObject!
Source file: chrome://flashblock/content/flashblock.xml
Line: 41

// Substitute the animation with a placeholder
function flashblockShowPlaceholder() {
 // Just in case the object has been moved away from under our feet during
 // the timeout, re-assign the parent node. See bug 13680
 parent = current.parentNode;
 parent.insertBefore(placeholder, current);
 if(placeholder.isStandalone) {
  placeholder.flashblock = "frame";
  current.StopPlay(); <-- Problem occurs **********
  current.LoadMovie(0, "");
  current.prevWidth = current.width; <-- This is OK.
  current.prevHeight = current.height;
  current.width = current.height = 0;
 } else {
  placeholder.flashblock = "normal";
  parent.removeChild(current);
 }
}

Sounds like a new bug... could you please file and attach a testcase? Thanks!