309044 - Flashplayer 8 "Bad NPObject as private data!"

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Philip Chee]

Error: [JavaScript Error: "Bad NPObject as private data!" {file:
"chrome://flashblock/content/flashblock.xml" line: 190}]
Source File: chrome://flashblock/content/flashblock.xml
Line: 190

The Flashblock extension triggers a bug in the plugin code when flashplayer 8
plugin is installed. This problem does not occur with flashplayer 7 is installed.

Problem appears in:
Firefox 1.0.6,
Firefox/1.4 (Gecko/20050917)
SeaMonkey/1.1a (Gecko/20050912)

Steps to reproduce:

1. Install flashplayer 8.0.22 from the Macromedia website.
   <http://www.macromedia.com/software/flashplayer/>
2. Install Flashblock 1.3.2 or later from:
   <http://flashblock.mozdev.org/installation.html>
3. Visit test case at:
   <http://bugzilla.mozdev.org/attachment.cgi?id=3443>
4. Click on flashblock placeholder to reveal flash.

Expected results: Flash is shown.

Actual results: Flashblock placeholder remains in place.
Error shows up in the javascript console.

We add a custom attribute "prevHeight" to the html flash element before removing
it from the DOM. When we restore the element we check for the existence of this
attribute and we are failing on this line:

if("prevHeight" in current) {

I tried using a try/catch block but for some reason the try/catch was not
catching this error.

current.prevHeight
current.getAttribute("prevHeight")

also cause this error.

Even if the bug is with the macromedia flash plugin, try/catch should have
caught it but doesn't.

Possibly related bug 308814

Comment 1

[Philip Chee]

Attachment: Flashblock.xml see line 190

Comment 2

[Martijn Wargers (dead)]

Attachment: flash file for testcase

Comment 3

[Martijn Wargers (dead)]

Attachment: 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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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....

Comment 6

[Henrik Gemal]

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

Comment 7

[Philip Chee]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Michelle Sintov]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Johnny Stenback (:jst)]

Attachment: 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 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201809 [details] [diff] [review]
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 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 201809 [details] [diff] [review]
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

r=mrbkap

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 201809 [details] [diff] [review]
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

sr=me.

/be

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 201809 [details] [diff] [review]
Take a plugin's scriptable object out of the plugin element's prototype chain on plugin destruction

Requesting approval for various branches...

Comment 16

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 17

[Philip Chee]

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.

Comment 18

[Marcia Knous [:marcia]]

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.

Comment 19

[Marcia Knous [:marcia]]

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.

Comment 20

[Johnny Stenback (:jst)]

Fix landed on the 1.8 branch.

Comment 21

[Scott MacGregor]

flag pixie dust

Comment 22

[Jerome Lacoste]

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

Comment 23

[Martijn Wargers (dead)]

(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.

Comment 24

[Henrik Gemal]

(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

Comment 25

[Martijn Wargers (dead)]

(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. 

Comment 26

[Boris Zbarsky [:bzbarsky]]

I think this caused bug 316025.

Comment 27

[Philip Chee]

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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).

Comment 30

[Philip Chee]

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..."

Comment 31

[Boris Zbarsky [:bzbarsky]]

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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix it

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

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 203609 [details] [diff] [review]
This should fix it

r+sr=jst

Comment 35

[Philip Chee]

> jst: approval1.8.0.1?

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 38

[Mike Schroepfer]

Comment on attachment 203609 [details] [diff] [review]
This should fix it

Please land on 1.8 and 1.8.1 branches.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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;

Comment 41

[Jay Patel [:jay]]

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.

Comment 42

[Philip Chee]

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?

Comment 43

[Boris Zbarsky [:bzbarsky]]

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

Comment 44

[Arnaud]

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 ?

Comment 45

[Boris Zbarsky [:bzbarsky]]

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

That's bug 90268.

Comment 46

[Philip Chee]

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); 
	}
}

Comment 47

[Boris Zbarsky [:bzbarsky]]

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