300756 - Flash 8 Player (Public Beta) crashes Deer Park [@ js_SetupLocks]

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Aronnax]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050713 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050713 Firefox/1.0+

Flash 8 crashes Deer Park very time the flash player dispatched data

Reproducible: Always

Steps to Reproduce:
1. install the Flash Player 8 Public Beta from:
http://www.macromedia.com/software/flashplayer/public_beta/
2. restart and go back to:
http://www.macromedia.com/software/flashplayer/public_beta/
3. click on (the blue text in the grey flash box): If you are having difficulty
with installation, please click here for more information
Actual Results:  
crash

Expected Results:  
open a new page

Comment 1

[Ria Klaassen (not reading all bugmail)]

It crashes also on WinXP: TB7475272W
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050714
Firefox/1.0+ ID:2005071403

Comment 2

[Jaime Mitchell (use [e-mail] for email)]

Please provide talkback ID's for the crashes.

Comment 3

[Aronnax]

works now, with a present day nightly - thanks ;-)

Comment 4

[Jaime Mitchell (use [e-mail] for email)]

Reopening, there is no evidence this was fixed, to my knowledge no relevant
changes were made. Resolving WFM based on comment 3. Please reopen if you find
the issue reoccurs.

Comment 5

[Aronnax]

(In reply to comment #4)
> Reopening, there is no evidence this was fixed, to my knowledge no relevant
> changes were made. Resolving WFM based on comment 3. Please reopen if you find
> the issue reoccurs.

sorry, works on the first try with a new nightly - on a second try he browser
craches again ;-(

Comment 6

[William Bumgarner [:zsinj]]

reopened as per comment #5

Comment 7

[Matthias Versen [:Matti]]

Does it also happen if you disable bfcache ?
The winxp stack from comment #1:
js_SetupLocks 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jslock.c, line 811]
JS_SetPrivate 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 2158]
nsWindowConstructor 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build/nsWinWidgetFactory.cpp,
line 65]
PresShell::ProcessReflowCommands 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6800]
nsBoxObject::QueryInterface 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxObject.cpp,
line 75]
PresShell::PostReflowEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6734]
DocumentViewerImpl::SetDOMDocument 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsDocumentViewer.cpp,
line 1556]
DocumentViewerImpl::SetEnableRendering 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsDocumentViewer.cpp,
line 1901]
Conv_06_FE_WithReverse 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsBidiUtils.cpp,
line 489]
PresShell::HandlePostedReflowCallbacks 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5241]
PresShell::ContentStatesChanged 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5421]
PL_HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/xpcom/threads/plevent.c,
line 686]
SHELL32.dll + 0x520c24 (0x778b0c24)

Comment 8

[Matthias Versen [:Matti]]

*** Bug 300776 has been marked as a duplicate of this bug. ***

Comment 9

[Matthias Versen [:Matti]]

bz: Do you have an idea where this shoould go to ?

Comment 10

[Jaime Mitchell (use [e-mail] for email)]

The stack in the duped bug (bug 300776) matches the one in bug 300572 the stack
below is different. 

This may be a dupe of bug 300572 , bug 300776 is a dupe of bug 300572

Comment 11

[Boris Zbarsky [:bzbarsky]]

I have no clue; the stack in comment 7 make no sense -- those functions don't
call each other.

Comment 12

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #7)
> Does it also happen if you disable bfcache ?

I get a crash every time with bfcache to 3 (default);
I get no crashes so far with bfcache to 0.

TB7498968H  TB7499076K  Sorry no links; the TB-server is too busy..

Comment 13

[Ria Klaassen (not reading all bugmail)]

Here they are:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=7498968H
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=7499076K
The two seem identical.

Comment 14

[Ria Klaassen (not reading all bugmail)]

Hm, this gets weird; I only need to click the button "Home" (or any other grey
button on top of this page) to get a crash:
http://www.macromedia.com/bin/fp8betafeedback.cgi?
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB7530381Y

Comment 15

[timeless]

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=js_setuplocks&vendor=All&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

11 crashes found where the Stack Signature contains 'js_setuplocks' and the sort
is by ID.

Comment 16

[Asa Dotzler [:asa]]

bryner, sounds like this might be bfcache releated. Can you take a look? 

This is way down on the topcrash list for DPA2, unlike bug 300572 which is right
at the top.

Comment 17

[Ria Klaassen (not reading all bugmail)]

It still crashes with Flash 8.0 r5. 
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB8186278G

Comment 18

[Mike Schroepfer]

Bryner - looks like it might be BFCache related - can you take a look?

Comment 19

[Brendan Eich [:brendan]]

Does this bug bite only if bfcache is enabled
(browser.sessionhistory.max_viewers non-zero)?  There don't seem to be any
comments testifying to that.

This could be a Flash 8 bug.  It could be a bfcache bug, but comment 0 says
nothing about going back as in using the Back button -- step 2 says "restart and
go back to ..." but that's just talking about loading the same URL again in a
new browser instance.

We shouldn't bug bryner with this.  I'll look into it.

/be

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #19)
> Does this bug bite only if bfcache is enabled
> (browser.sessionhistory.max_viewers non-zero)?  There don't seem to be any
> comments testifying to that.
 
Comment 12 does, and it's my experience as well that disabling bfcache makes the
Flash crasher go away.

> This could be a Flash 8 bug.  It could be a bfcache bug, but comment 0 says
> nothing about going back as in using the Back button -- step 2 says "restart and
> go back to ..." but that's just talking about loading the same URL again in a
> new browser instance.
> 
> We shouldn't bug bryner with this.  I'll look into it.
> 
> /be

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #7)
> > Does it also happen if you disable bfcache ?
> 
> I get a crash every time with bfcache to 3 (default);
> I get no crashes so far with bfcache to 0.
> 
> TB7498968H  TB7499076K  Sorry no links; the TB-server is too busy..

Duh, sorry I missed this -- I was find-as-you-type'ing "bfcache", not sure why
that didn't work.

Anyway, I'll look into this.  It sounds like some plugin API calling protocol
(maybe to do with SetWindow) got broken by the bfcache changes.  Cc'ing jst, and
if anyone knows of another bug about SetWindow regressions, please comment here.

/be

Comment 22

[Ria Klaassen (not reading all bugmail)]

Yes, it crashes only with bfcache enabled.

Comment 23

[Brendan Eich [:brendan]]

Need to dig more, hard to estimate ETA.  Will get together with jst for a plugin
API consultation and some hypotheses about the bug based on the crash traces
when he has more time, so next week.

/be

Comment 24

[Ria Klaassen (not reading all bugmail)]

Sorry sorry, for being premature, the original testcase does not crash anymore
on my computer with Flash 8.0.15.0.

A similar new case (also bfcache) is discussed here:
http://forums.mozillazine.org/viewtopic.php?p=1685904#1685904
The talkback looks (for me) similar to the one I posted in comment #17.

Talkback: TB8524760Z  TB8529060Z

Testcase: go to http://www.macromedia.com/
Enter into the search box: beta
Click on Search button. 

Comment 25

[Brendan Eich [:brendan]]

Stephend reproduced under VS.NET 2003 and showed the same stack as talkback:

<[1]stephend> > gkplugin.dll!_releaseobject(NPObject * npobj=0x04f420e8)  Line
1525 C++
<[1]stephend>   gkplugin.dll!NPObjWrapper_Finalize(JSContext * cx=0x0251ee08,
JSObject * obj=0x01c47db0)  Line 1195 + 0x6 C++
<[1]stephend>   js3250.dll!js_FinalizeObject(JSContext * cx=0x0251ee08, JSObject
* obj=0x01c47db0)  Line 2074 C
<[1]stephend>   js3250.dll!js_GC(JSContext * cx=0x0251ee08, unsigned int
gcflags=0)  Line 1839 + 0x9 C
<[1]stephend>   js3250.dll!js_ForceGC(JSContext * cx=0x0251ee08, unsigned int
gcflags=0)  Line 1510 + 0x1f C
<[1]stephend>   js3250.dll!JS_GC(JSContext * cx=0x0251ee08)  Line 1843 + 0x8 C
<[1]stephend>   gklayout.dll!nsJSContext::Notify(nsITimer * timer=0x029dcd10) 
Line 2156 C++
<[1]stephend>   xpcom_core.dll!nsTimerImpl::Fire()  Line 417 C++
<[1]stephend>   xpcom_core.dll!nsTimerManager::FireNextIdleTimer()  Line 628 C++
<[1]stephend>   gkwidget.dll!nsAppShell::Run()  Line 142 C++
<brendan> what is *obj
<stephend> - map 0x02dc6c88 {nrefs=1 ops=0x00cda480 _js_ObjectOps nslots=5 ...}
JSObjectMap *
<stephend>  nrefs 1 long
<stephend> + ops 0x00cda480 _js_ObjectOps JSObjectOps *
<stephend>  nslots 5 unsigned long
<stephend>  freeslot 4 unsigned long
<stephend> + slots 0x00ab9d94 long *
<brendan> stephend: *(JSClass*)(obj->slots[2]-1)
<stephend> - *(JSClass*)(obj->slots[2]-1) {name=0x00f36c38 "NPObject JS wrapper
class" flags=5 addProperty=0x00f2bcfd NPObjWrapper_AddProperty(JSContext *,
JSObject *, long, long *) ...} JSClass

Unfortunately, he couldn't get to the NPObjWrapper_Finalize or _releaseobject
frames, to confirm that obj's private data (of type NPObject) had already been
freed.  But that seems to be what happened.

This suggests the bug could be at least partly on the plugin's side.  If our
reference-counting of the NPObject is correct, then this looks like the plugin
double-dropped, or failed to hold when it should have.  Flash folks: is this
something you could check or double-check?  Thanks,

/be

Comment 26

[Johnny Stenback (:jst)]

Attachment: Make sure we find cx so we can clear private data when destroying plugin

When using bfcache we freeze the presshell when leaving a page, doing that ends
up tearing down the plugins in a window of time when a script global object is
not reachable from the document. Because of this, we won't find the JS context
for a plugin when we're invalidating the JS objects that wrap NPObjects from
the plugin, so we fail to clear their private data. The NPObjects are
invalidated, and thus freed etc, so we'll later on end up stomping all over
freed memory etc, which is likely to lead us to double releasing window objects
etc.

This fixes that problem by making GetJSContext() in nsJSNPRuntime.cpp use a
more reliable way to get to the JS context from a document.

Comment 27

[Mike Shaver (:shaver emeritus)]

Comment on attachment 193458 [details] [diff] [review]
Make sure we find cx so we can clear private data when destroying plugin 

sr=shaver, though I do wonder why doc->GetScriptGlobalObject doesn't do the
same thing as you're doing here.  I wonder idly, though; don't feel compelled
to explain!

Comment 28

[Boris Zbarsky [:bzbarsky]]

Doesn't bfcache also set the container on the document to null, though?  So
we're depending on some sort of timing thing here?

It might be better for 1.9 to move ahead with making GetScriptGlobalObject never
return null and then backing out this change, since at that point using
GetScriptGlobalObject will be far less fragile.

Comment 29

[Johnny Stenback (:jst)]

(In reply to comment #27)
> (From update of attachment 193458 [details] [diff] [review] [edit])
> sr=shaver, though I do wonder why doc->GetScriptGlobalObject doesn't do the
> same thing as you're doing here.  I wonder idly, though; don't feel compelled
> to explain!

Just to put the record staight, the reason this happens is that we end up
calling SetScriptGlobalObject(nsnull) on the document when we leave the page,
then we call Freeze() on the pres shell, and that's where we end up tearing down
the plugin stuff. After this, the document's mScriptGlobalObject is null, but
mIsGoingAway false, so GetScriptGlobalObject() will return null.

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 193458 [details] [diff] [review]
Make sure we find cx so we can clear private data when destroying plugin 

r+a=me, thanks for fixing this.  Leave the bug open for a better trunk patch,
or file a followup bug?

/be

Comment 31

[Brendan Eich [:brendan]]

*** Bug 300572 has been marked as a duplicate of this bug. ***

Comment 32

[Johnny Stenback (:jst)]

Fixed on the branch.

Comment 33

[Zulith]

This checkin seems to have caused a regression in the behavior of Adblock and
the unofficial Adblock plus extensions.

Using the following testcase:
Go to www.macromedia.com
Entered into the search box: beta
Click "Search"
hit back button

All flash objects have now disappeared on the front page of macromedia.com

Disabling Adblock from the extension manager fixes this. Please note that the
Adblock filter in use (Filterset.G) is not blocking the flash objects on this page.

Further commentary here:
http://forums.mozillazine.org/viewtopic.php?p=1693080#1693080

Comment 34

[Boris Zbarsky [:bzbarsky]]

That's VERY unlikely to be a result of this checkin.  What's the date range on
that regression?

Comment 35

[Boris Zbarsky [:bzbarsky]]

And I would recommend filing a separate bug on the regression.

Comment 36

[Michelle Sintov]

We also had to fix some Flash Player issues in order to fix this bug. The fixes
are in Flash Player 8r19, which is not yet publicly available.

Comment 37

[Mike Connor [:mconnor]]

*** Bug 303811 has been marked as a duplicate of this bug. ***

Comment 38

[Brendan Eich [:brendan]]

(In reply to comment #28)
> Doesn't bfcache also set the container on the document to null, though?  So
> we're depending on some sort of timing thing here?
> 
> It might be better for 1.9 to move ahead with making GetScriptGlobalObject never
> return null and then backing out this change, since at that point using
> GetScriptGlobalObject will be far less fragile.

We should do this, but if not soon, then I say land the branch patch on the
trunk to avoid the crash and talkback noise.  jst, what do you say?

/be

Comment 39

[Boris Zbarsky [:bzbarsky]]

In case I was unclear, I'm all for fixing this bug on trunk with the patch we
have.  As long as we file a followup bug to make the world a better place.

Comment 40

[Boris Zbarsky [:bzbarsky]]

OK, I checked in that patch on the trunk and filed bug 308902 on the issue of
mScriptGlobalObject immutability.

Comment 41

[Stephen Donner [:stephend] Not actively reading bugmail]

I've run the full gamut of my usual Flash-powered websites, and have had _zero_
Flash crashes using:

SeaMonkey 1.1a:Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050917 Mozilla/1.0

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917
Firefox/1.6a1

Verified FIXED on trunk; thanks!

Comment 42

[Jay Patel [:jay]]

v.fixed on branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20050928 Firefox/1.4 and Shockwave Flash 8.0 r22, no crashes with urls in
this bug and other Flash content I see daily.

Comment 43

[Johnny Stenback (:jst)]

*** Bug 304351 has been marked as a duplicate of this bug. ***