Last Comment Bug 398668 - [FIX]Crash [@ JS_GetPrivate] with binding with destructor, setting javascript disabled, reloading and going back
: [FIX]Crash [@ JS_GetPrivate] with binding with destructor, setting javascript...
: crash, regression, testcase, verified1.8.1.12
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86 Windows XP
: P1 critical (vote)
: mozilla1.9beta1
Assigned To: Boris Zbarsky [:bz]
Depends on:
  Show dependency treegraph
Reported: 2007-10-04 17:40 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-09 14:58 PDT (History)
6 users (show)
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

zipped testcase (943 bytes, application/zip)
2007-10-04 17:40 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Fix (8.21 KB, patch)
2007-10-05 16:30 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
jst: approval1.9+
Details | Diff | Splinter Review
Fix build bustage: mIsCompiled is debug-only (2.88 KB, patch)
2007-10-05 18:23 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Branch patch (11.46 KB, patch)
2007-10-05 18:53 PDT, Boris Zbarsky [:bz]
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-04 17:40:59 PDT
Created attachment 283645 [details]
zipped testcase

See zipped up testcase.

To reproduce the crash, unzip it and then open up parentframe2.htm
It should crash current trunk build within 1 second.

This seems to have regressed between 2007-07-11 and 2007-07-12:
0  	JS_GetPrivate  	 mozilla/js/src/jsapi.c:2799
1 	nsScriptSecurityManager::GetFunctionObjectPrincipal(JSContext*, JSObject*, JSStackFrame*, unsigned int*) 	mozilla/caps/src/nsScriptSecurityManager.cpp:2138
2 	nsScriptSecurityManager::CheckFunctionAccess(JSContext*, void*, void*) 	mozilla/caps/src/nsScriptSecurityManager.cpp:1618
3 	nsXBLProtoImplAnonymousMethod::Execute(nsIContent*) 	mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp:347
4 	nsXBLBinding::ExecuteDetachedHandler() 	mozilla/content/xbl/src/nsXBLBinding.cpp:950
5 	nsBindingManager::ExecuteDetachedHandlers() 	mozilla/content/xbl/src/nsBindingManager.cpp:878
6 	nsGlobalWindow::PostHandleEvent(nsEventChainPostVisitor&) 	mozilla/dom/src/base/nsGlobalWindow.cpp:1964
7 	nsEventTargetChainItem::PostHandleEvent(nsEventChainPostVisitor&) 	mozilla/content/events/src/nsEventDispatcher.cpp:218
8 	nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:267
9 	nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:316
10 	nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:479
11 	DocumentViewerImpl::PageHide(int) 	mozilla/layout/base/nsDocumentViewer.cpp:1156
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-10-05 13:33:29 PDT
Hmm.. nothing is standing out in that range :(
Comment 2 Boris Zbarsky [:bz] 2007-10-05 14:03:52 PDT
###!!! ASSERTION: SHEntry already contains viewer: '!aViewer || !mContentViewer', file ../../../../mozilla/docshell/shistory/src/nsSHEntry.cpp, line 227

The stack shows us killing off an old viewer from the Show() for a new viewer, triggered from a reflow event calling UnsuppressAndInvalidate.

Offhand, bug 364461 could be responsible here.  Although I tried backing that out and I still get that assert.  I think that's the real issue here, in any case.
Comment 3 Boris Zbarsky [:bz] 2007-10-05 14:08:06 PDT
Though backing out that change _does_ fix the crash.

When we do crash, we're doing a security check for XBL stuff, after some other asserts fired, and we have a JSObject which has 0x4 as its JSClass*.  So it's just dead.
Comment 4 Boris Zbarsky [:bz] 2007-10-05 16:30:32 PDT
Created attachment 283780 [details] [diff] [review]

This is great fun.  There were three things wrong here, really:

1)  Clobbering of the existing document viewer.  Not really a cause of this bug.
2)  Clobbering of the "javascript disabled" state on the docshell on bfcache
3)  XBL not handling a situation where script is disabled at binding attachment
    time and then gets enabled before destructor time.  We tried to treat a
    string as a JavaScript object, which clearly doesn't work.

This patch fixes all three.
Comment 5 Boris Zbarsky [:bz] 2007-10-05 16:34:11 PDT
I think we want all those changes on branch too; all of those are problems there as well.
Comment 6 Boris Zbarsky [:bz] 2007-10-05 16:51:28 PDT
Comment on attachment 283780 [details] [diff] [review]

Requesting trunk approval
Comment 7 Boris Zbarsky [:bz] 2007-10-05 18:23:38 PDT
Created attachment 283795 [details] [diff] [review]
Fix build bustage: mIsCompiled is debug-only
Comment 8 Boris Zbarsky [:bz] 2007-10-05 18:47:12 PDT
Checked in on trunk.  Would be nice to get that testcase into a testsuite too....
Comment 9 Boris Zbarsky [:bz] 2007-10-05 18:53:50 PDT
Created attachment 283798 [details] [diff] [review]
Branch patch

This should be fairly safe, in my opinion/
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-07 11:05:53 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100705 Minefield/3.0a9pre
Comment 11 Daniel Veditz [:dveditz] 2007-11-13 11:11:04 PST
Is this appropriate for a rushed I'm thinking not but willing to hear arguments for safety and ease of regression testing.

Do we know what this regressed from?
Comment 12 Boris Zbarsky [:bz] 2007-11-13 11:57:03 PST
> Is this appropriate for a rushed

Probably not.  This is not very safe, not very easily testable code.  :(

> Do we know what this regressed from?

Changes in memory layout, as far as I can tell.  The actual problems the patch is fixing have been in place for a long time: items 1 and 2 ever since bfcache landed, and item 3 probably at least since Gecko 1.8.0, possibly since 1.7.0 or earlier.
Comment 13 Daniel Veditz [:dveditz] 2007-11-13 12:07:39 PST
Comment on attachment 283798 [details] [diff] [review]
Branch patch is a better spot for patches we're not totally confident are risk-free.
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-11-13 15:35:27 PST
Maybe this bug should not be fixed at all on branch?
Comment 15 Boris Zbarsky [:bz] 2007-11-13 17:25:39 PST
I think it should.  Some of the possible crashes here are pretty nasty.
Comment 16 Daniel Veditz [:dveditz] 2007-12-21 11:22:13 PST
Comment on attachment 283798 [details] [diff] [review]
Branch patch

approved for, a=dveditz for release-drivers
Comment 17 Boris Zbarsky [:bz] 2008-01-11 12:01:49 PST
Fixed on branch.
Comment 18 Al Billings [:abillings] 2008-01-18 12:38:08 PST
Verified on branch. No crash anymore.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2008011803 BonEcho/

Note You need to log in before you can comment on or make changes to this bug.