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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Benjamin Smedberg]

Going to mark hardblocker for now, bz feel free to re-mark if this isn't so serious.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 (??).

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Safety patch

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

To be clear, I think we should take that patch no matter what, even if we find the core of the SHistory issue.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Yeah, makes sense.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Safer nsDocShell::CanSavePresentation

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).

Comment 10

[Boris Zbarsky [: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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I'll push both patches soon.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/ebb58e2a3b29
http://hg.mozilla.org/mozilla-central/rev/c9f50a19849e