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

VERIFIED FIXED in mozilla1.9beta1

Status

()

Core
XBL
P1
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(4 keywords)

Trunk
mozilla1.9beta1
x86
Windows XP
crash, regression, testcase, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.8.1.12 +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

10 years ago
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:
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...
Hmm.. nothing is standing out in that range :(
Flags: blocking1.9?
###!!! 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.
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.
Created attachment 283780 [details] [diff] [review]
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #283780 - Flags: superreview?(jst)
Attachment #283780 - Flags: review?(jst)
I think we want all those changes on branch too; all of those are problems there as well.
Flags: blocking1.8.1.9?
Priority: -- → P1
Summary: Crash [@ JS_GetPrivate] with binding with destructor, setting javascript disabled, reloading and going back → [FIX]Crash [@ JS_GetPrivate] with binding with destructor, setting javascript disabled, reloading and going back
Target Milestone: --- → mozilla1.9 M9

Updated

10 years ago
Attachment #283780 - Flags: superreview?(jst)
Attachment #283780 - Flags: superreview+
Attachment #283780 - Flags: review?(jst)
Attachment #283780 - Flags: review+
Comment on attachment 283780 [details] [diff] [review]
Fix

Requesting trunk approval
Attachment #283780 - Flags: approval1.9?

Updated

10 years ago
Attachment #283780 - Flags: approval1.9? → approval1.9+
Created attachment 283795 [details] [diff] [review]
Fix build bustage: mIsCompiled is debug-only
Checked in on trunk.  Would be nice to get that testcase into a testsuite too....
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Created attachment 283798 [details] [diff] [review]
Branch patch

This should be fairly safe, in my opinion/
Attachment #283798 - Flags: approval1.8.1.9?
(Reporter)

Comment 10

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100705 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.10?
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?
> 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.
Flags: blocking1.8.1.10?
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.
Attachment #283798 - Flags: approval1.8.1.10? → approval1.8.1.11?
(Reporter)

Comment 14

10 years ago
Maybe this bug should not be fixed at all on branch?
I think it should.  Some of the possible crashes here are pretty nasty.
Comment on attachment 283798 [details] [diff] [review]
Branch patch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #283798 - Flags: approval1.8.1.12? → approval1.8.1.12+
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Flags: wanted1.8.1.x+
Fixed on branch.
Keywords: fixed1.8.1.12
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
Keywords: fixed1.8.1.12 → verified1.8.1.12

Updated

8 years ago
Flags: blocking1.9?
Crash Signature: [@ JS_GetPrivate]
You need to log in before you can comment on or make changes to this bug.