Closed
Bug 309044
Opened 19 years ago
Closed 19 years ago
Flashplayer 8 "Bad NPObject as private data!"
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: philip.chee, Assigned: jst)
References
()
Details
(Keywords: fixed1.8.1, testcase, verified1.8.0.1)
Attachments
(5 files)
10.13 KB,
text/plain
|
Details | |
7.93 KB,
application/x-shockwave-flash
|
Details | |
622 bytes,
text/html
|
Details | |
3.63 KB,
patch
|
mrbkap
:
review+
brendan
:
superreview+
dveditz
:
approval-aviary1.0.8-
dveditz
:
approval1.7.13-
mtschrep
:
approval1.8rc2+
|
Details | Diff | Splinter Review |
2.68 KB,
patch
|
jst
:
review+
jst
:
superreview+
mtschrep
:
approval1.8.0.1+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•19 years ago
|
||
Comment 2•19 years ago
|
||
Comment 3•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
aren't this a blocker for 1.5? flash 8 objects are potential useless because of this.
Reporter | ||
Comment 7•19 years ago
|
||
Adding Michelle Sintov of Macromedia to the CC list in the hopes that she can shed some light on this bug.
Comment 8•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 11•19 years ago
|
||
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.
Attachment #201809 -
Flags: superreview?(brendan)
Attachment #201809 -
Flags: review?(mrbkap)
Assignee | ||
Updated•19 years ago
|
Assignee: nobody → jst
OS: Windows XP → All
Hardware: PC → All
Comment 12•19 years ago
|
||
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•19 years ago
|
||
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
Attachment #201809 -
Flags: review?(mrbkap) → review+
Comment 14•19 years ago
|
||
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
Attachment #201809 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 15•19 years ago
|
||
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...
Attachment #201809 -
Flags: approval1.8rc2?
Attachment #201809 -
Flags: approval1.7.13?
Attachment #201809 -
Flags: approval-aviary1.0.8?
Assignee | ||
Comment 16•19 years ago
|
||
Fixed on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 17•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #201809 -
Flags: approval1.8rc2? → approval1.8rc2+
Comment 18•19 years ago
|
||
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•19 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 22•19 years ago
|
||
I agree with Henrik: will this be fixed in 1.5? From reading the comments, I don't get that impression.
Comment 23•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
I think this caused bug 316025.
Reporter | ||
Comment 27•19 years ago
|
||
I'm still seeing this on Firefox 1.5RC3. Did somebody forget to land this on the Gecko 1.8 branch?
Comment 28•19 years ago
|
||
No, this was checked in on the branch. Check the CVS logs if you want.
Comment 29•19 years ago
|
||
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).
Reporter | ||
Comment 30•19 years ago
|
||
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..."
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 31•19 years ago
|
||
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?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Comment 32•19 years ago
|
||
Makes sure to look in the right scope... That doesn't matter for WrapNative, but does for GetWrappedNativeOfNativeObj, unfortunately.
Attachment #203609 -
Flags: superreview?(jst)
Attachment #203609 -
Flags: review?(jst)
Comment 33•19 years ago
|
||
That patch fixes the testcase for me, day old Windows branch build w/Flash 8. No JS Console error, reappears as described.
Assignee | ||
Comment 34•19 years ago
|
||
Comment on attachment 203609 [details] [diff] [review]
This should fix it
r+sr=jst
Attachment #203609 -
Flags: superreview?(jst)
Attachment #203609 -
Flags: superreview+
Attachment #203609 -
Flags: review?(jst)
Attachment #203609 -
Flags: review+
Attachment #203609 -
Flags: approval1.8.0.1?
Reporter | ||
Comment 35•19 years ago
|
||
> jst: approval1.8.0.1?
Does this mean that the fix won't make Firefox 1.5 final?
Comment 36•19 years ago
|
||
Probably not, no. 1.5 final is in code freeze at this point.
Comment 37•19 years ago
|
||
Fixed on trunk.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Comment 38•19 years ago
|
||
Comment on attachment 203609 [details] [diff] [review]
This should fix it
Please land on 1.8 and 1.8.1 branches.
Attachment #203609 -
Flags: approval1.8.0.1? → approval1.8.0.1+
Comment 39•19 years ago
|
||
Could someone please land that last patch on branch? If not, I'll do it in early January...
Comment 40•19 years ago
|
||
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;
Keywords: fixed1.8.0.1,
fixed1.8.1
Updated•19 years ago
|
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
Comment 41•19 years ago
|
||
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.
Keywords: fixed1.8.0.1 → verified1.8.0.1
Updated•19 years ago
|
Attachment #201809 -
Flags: approval1.7.13?
Attachment #201809 -
Flags: approval1.7.13-
Attachment #201809 -
Flags: approval-aviary1.0.8?
Attachment #201809 -
Flags: approval-aviary1.0.8-
Reporter | ||
Comment 42•19 years ago
|
||
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•19 years ago
|
||
You'd have to address that to drivers...
Comment 44•19 years ago
|
||
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•19 years ago
|
||
> If I set style.display="none" on the plugin
That's bug 90268.
Reporter | ||
Comment 46•19 years ago
|
||
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•19 years ago
|
||
Sounds like a new bug... could you please file and attach a testcase? Thanks!
See Also: → https://launchpad.net/bugs/128062
Reporter | ||
Updated•14 years ago
|
See Also: https://launchpad.net/bugs/128062 →
See Also: → https://launchpad.net/bugs/128062
Reporter | ||
Updated•14 years ago
|
See Also: https://launchpad.net/bugs/128062 →
See Also: → https://launchpad.net/bugs/128062
Reporter | ||
Updated•14 years ago
|
See Also: https://launchpad.net/bugs/128062 →
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•