398668 - [FIX]Crash [@ JS_GetPrivate] with binding with destructor, setting javascript disabled, reloading and going back

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: 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:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-11+04&maxdate=2007-07-12+09&cvsroot=%2Fcvsroot

http://crash-stats.mozilla.com/report/index/1fb9fe8f-72d8-11dc-bd44-001a4bd43ef6
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
etc...

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. nothing is standing out in that range :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

###!!! 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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Fix

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
    restore.
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 [:bzbarsky]]

I think we want all those changes on branch too; all of those are problems there as well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283780 [details] [diff] [review]
Fix

Requesting trunk approval

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix build bustage: mIsCompiled is debug-only

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  Would be nice to get that testcase into a testsuite too....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

This should be fairly safe, in my opinion/

Comment 10

[Martijn Wargers (dead)]

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

Is this appropriate for a rushed 1.8.1.10? 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 [:bzbarsky]]

> Is this appropriate for a rushed 1.8.1.10?

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

Comment on attachment 283798 [details] [diff] [review]
Branch patch

1.8.1.11 is a better spot for patches we're not totally confident are risk-free.

Comment 14

[Martijn Wargers (dead)]

Maybe this bug should not be fixed at all on branch?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I think it should.  Some of the possible crashes here are pretty nasty.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 283798 [details] [diff] [review]
Branch patch

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified on branch. No crash anymore.

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