Closed Bug 633413 Opened 12 years ago Closed 12 years ago

"ASSERTION: Wrong scope, this is really bad!"


(Core :: DOM: Navigation, defect)

Windows 7
Not set



Tracking Status
blocking2.0 --- final+


(Reporter: jruderman, Assigned: bzbarsky)


(Keywords: assertion, testcase, Whiteboard: [need landing][sg:critical?][hardblocker][has patch])


(4 files)

Attached file testcase
###!!! ASSERTION: SHEntry already contains viewer: '!aViewer || !mContentViewer', file docshell/shistory/src/nsSHEntry.cpp, line 242

###!!! ASSERTION: Wrong scope, this is really bad!: 'JS_GetGlobalForObject(cx, obj) == newScope', file content/base/src/nsDocument.cpp, line 3807

Seems similar to bug 622480, which was fixed last month.  Assuming it's equally severe.
Attached file stack traces
blocking2.0: --- → ?
OS: Mac OS X → Windows 7
Going to mark hardblocker for now, bz feel free to re-mark if this isn't so serious.
Assignee: nobody → bzbarsky
blocking2.0: ? → final+
Whiteboard: [sg:critical?] → [sg:critical?][hardblocker]
OK, so basic data.  

When that first assert above files, the old viewer is for the url "data:text/html,2" and the new one is for "file:///Users/bzbarsky/test.html#a" if I put this testcase in ~/test.html.

When the second assert fires we're in the middle of nsDocShell::RestoreFromHistory for the "data:text/html,2" url.  We have this stack:

#1  0x0065f0da in nsDocument::SetScriptGlobalObject (this=0x2bbb3200, aScriptGlobalObject=0x171b3ad8) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:3806
#2  0x009e6397 in nsGlobalWindow::SetNewDocument (this=0x27987060, aDocument=0x2bbb3200, aState=0x0, aForceReuseInnerWindow=0) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsGlobalWindow.cpp:2167
#3  0x003282ce in DocumentViewerImpl::InitInternal (this=0x25ace760, aParentWidget=0x0, aState=0x0, aBounds=@0x25ace7b4, aDoCreation=0, aNeedMakeCX=1, aForceSetNewDocument=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsDocumentViewer.cpp:968
#4  0x00328af1 in DocumentViewerImpl::Open (this=0x25ace760, aState=0x0, aSHEntry=0x239c16d0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsDocumentViewer.cpp:1367
#5  0x0108388b in nsDocShell::RestoreFromHistory (this=0x26105000) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:7097

Note that Open() calls InitInternal with aForceSetNewDocument set to the default value (true).  So we do a SetNewDocument on the outer window under here.  At this point the current document of the window is "data:text/html,3".  We clearly don't want to reuse the inner window in that case, so we end up creating a new inner because, and this is important, the window state is null here, so we don't have an existing inner to use.

The document passed to aForceSetNewDocument here has a null mScriptGlobalObject and null mScriptObject, but mHasHadScriptHandlingObject is true (??).
Er, I lied.  On the first assert the _new_ viewer is for "data:text/html,2" and the old one is for "file:///Users/bzbarsky/test.html#a".

So what happens is that the first assert firing is the real issue.  We end up calling SetContentViewer when a previous viewer is destroyed; it does this on the wrong shentry for some reason.  Presumably due to all the racy loads here... smaug, do you want to look into that part, or should I?

But once that happens, we throw away the old viewer and window state, set the new viewer but NOT a window state, and move on.  Then when we restore we have no window state, so we lose.
Attached patch Safety patchSplinter Review
This makes sure that we can't end up restoring without a WindowState.  This should be very safe (worst case prevents bfcache from happening in a few cases), and I think we should take this no matter what.  With this patch we still get the first assert from comment 0 and then some document viewer destruction asserts that obviously follow because we lost a viewer.  I'll look into those on Monday, but unless the fix there is really simple or Olli thinks those asserts are dangerous, I'd sort of prefer to not mess with session history before ship at this point.
Attachment #511922 - Flags: review?(Olli.Pettay)
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker][has patch]
Comment on attachment 511922 [details] [diff] [review]
Safety patch

We could take this, but I could perhaps debug this a bit later 
today or tomorrow.
Attachment #511922 - Flags: review?(Olli.Pettay) → review+
To be clear, I think we should take that patch no matter what, even if we find the core of the SHistory issue.
Whiteboard: [sg:critical?][hardblocker][has patch] → [need landing][sg:critical?][hardblocker][has patch]
Yeah, makes sense.
I'm still debugging the real cause for this bug,
but this kind of patch should catch several similar kinds of problems.
The warning is there for a reason. It shows that some code somewhere
doesn't behave the way it should, but we still handle that case (so no assertion).
Attachment #512216 - Flags: feedback?(bzbarsky)
Comment on attachment 512216 [details] [diff] [review]
Safer nsDocShell::CanSavePresentation

This seems reasonable, yeah.

I'd say that between this and the other safety patch we should call this bug fixed, and spin off the rest of the work to a different bug.
Attachment #512216 - Flags: review+
Attachment #512216 - Flags: feedback?(bzbarsky)
Attachment #512216 - Flags: feedback+
I'll push both patches soon.
blocking2.0: final+ → ?
blocking2.0: ? → final+
Group: core-security
You need to log in before you can comment on or make changes to this bug.