554 bytes, text/html
3.22 KB, text/plain
1.77 KB, patch
|Details | Diff | Splinter Review|
1.04 KB, patch
|Details | Diff | Splinter Review|
Created attachment 511594 [details] 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.
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.
Created attachment 511922 [details] [diff] [review] 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.
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.
Created attachment 512216 [details] [diff] [review] 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 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.
I'll push both patches soon.
blocking2.0: final+ → ?
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.