894215 - "Assertion failure: window" [@ nsDOMWindowList::IndexedGetter] with adoptNode, navigation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: window, at dom/base/nsDOMWindowList.cpp:102

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Attachment: alt stack

Without the last part of the testcase, it usually hits the same assert from ssi_getSelectedPageStyle in SessionStore.jsm.  But that's on a longer timer.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrm.  We can add a null-check, but why do we have a child docshell that has a null window???

Comment 4

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

So we're about to destroy the docshell we access, so GetInterface returns null.
But why do we have such docshell as a child when we call w[0]...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right, that was the question.  Why are we about to destroy a docshell that we have as a child?

Comment 6

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

Does anyone have any suggestion of a security rating for this?

Comment 7

[Andrew McCreight [:mccr8]]

Olli, should this be sec-moderate until we know that this is anything besides a null crash?

Comment 8

[Andrew McCreight [:mccr8]]

I'm going to reduce this to sec-moderate as it looks like just a null deref.  Feel free to increase it if you want.

Comment 9

[Bobby Holley (:bholley)]

This doesn't crash for me anymore, so I'd hazard a guess that this was fixed by Window WebIDL or something similar. Jesse, can you confirm that the testcase no longer works?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> so I'd hazard a guess that this was fixed by Window WebIDL or something similar

Unlikely.  The stack shown is the one we'd still take for indexed access on a window, and the same MOZ_ASSERT is still there...

Comment 11

[Jesse Ruderman]

I can still reproduce this bug.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This still reproduces on trunk. Note that the testcase requires you to set the "security.data_uri.unique_opaque_origin" and "security.data_uri.block_toplevel_data_uri_navigations" prefs to false now due to the restrictions on data URIs that were put in place.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, so I think I know what's going on.  The sequence of events is as follows:

1) We load a page, with a subframe.

2) We start loading a new page.

3) We call nsDocShell::Embed on the new page's content viewer.  This calls
   SetupNewViewer, which calls nsDocShell::DestroyChildren etc.  This nulls
   out the parent pointer on the subframe docshell but it's still around,
   because we are planning to put the first page into bfcache.  It _also_
   captures the list of child docshells and stores it in the SHEntry
   (see nsDocShell::CaptureState).

4) This code runs:

    document.adoptNode(page1root);

   The adoptNode call removes page1root from the DOM of its document,
   which is the document that we are navigating away from.  At this point
   it has _not_ yet been "fully" placed in bfcache, because Destroy() hasn't
   been called on the viewer.  Specifically, the SHEntry has been stored
   on the documentviewer, but we have not called
   nsSHEntryShared::SetContentViewer yet.  So we are not yet watching for mutations
   to drop the document from bfcache.  Before returning from the adoptNode call,
   we end up calling Destroy() on the subframe's docshell (via
   nsDocument::MaybeInitializeFinalizeFrameLoaders being called from a
   scriptblocker coming off the stack).  This nulls out its mScriptGlobal pointer,
   which is why we have a docshell that has a null window.

5) This code runs:

    w.history.back();

   When back() is called, that lands in nsDocShell::InternalLoad, which does:

           nsCOMPtr<nsISHEntry> viewerEntry;
           prevViewer->GetHistoryEntry(getter_AddRefs(viewerEntry));
           if (viewerEntry == aSHEntry) {
             // Make sure this viewer ends up in the right place
             mContentViewer->SetPreviousViewer(nullptr);
             prevViewer->Destroy();
           }

   This Destroy() call places prevViewer's document in the bfcache.  Then we
   keep going with the back() navigation and load it from bfcache.

6) When going back via bfcache we grab the set of child shells from the shentry.
   This was stored back in step 3, so contains the now-torn-down docshell.

Upshot: The fix for bug 292962 didn't actually fix it very well.  There's this time period while a new page is loading when we have already grabbed some bfcache state but don't have the mutation observers set up yet, and if the old page is mutated during this time, bad things will happen.

At least the "holds weak refs to docshells" bit got fixed in bug 319551, so we get a null-deref here, not a UAF....

Anyway, we should fix things so we drop bfcache in this case.  That _may_ be enough, since even nsSHEntryShared::RemoveFromBFCacheAsync (which is what we do on mutations) calls DropPresentationState() sync unless it fails to dispatch the runnable.  And DropPresentationState() is what clears out the child shell list in the shentry.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase using blob URIs instead of data:, for simplicity

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change nsSHEntryShared to inherit from nsStubMutationObserver

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make sure we don't bfcache pages that get mutated after we capture their state

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8976779 [details] [diff] [review]
part 2.  Make sure we don't bfcache pages that get mutated after we capture their state

This part still has some issues I'm investigating...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Fix the document "disallow bfcaching" mechanism to work without asserting

There are two issues being fixed here.  First, if DisallowBFCaching is called
before CanSavePresentation, we should really return false from
CanSavePresentation.  Otherwise we'll end up doing a bunch of state-capturing
work for no reason.

Second, if DisallowBFCaching is called between CanSavePresentation and
nsDocumentViewer::Destroy, we need to actually tear down the viewer state.
What we do right now is avoid putting the viewer into the SHEntry, but still
not tear down its presshell and so forth, which leads to asserts in
~nsDocumentViewer when this case is hit.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make sure we don't bfcache pages that get mutated after we capture their state

Comment 20

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

Comment on attachment 8977079 [details] [diff] [review]
part 2.  Fix the document "disallow bfcaching" mechanism to work without asserting

ahaa, 8 line context wasn't enough for DocumentViewer.

Comment 21

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

Comment on attachment 8977080 [details] [diff] [review]
part 3.  Make sure we don't bfcache pages that get mutated after we capture their state

I wonder if we could have reused the MutationObserver session history has. But this isn't too complicated either.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> I wonder if we could have reused the MutationObserver session history has.

Hmm.  I guess with the new setup where I just disallow bfcaching on the document (as opposed to the earlier attempt where it did something else) we could do that...  The logic might still end up more complicated and harder to follow, though.  In particular, we would need to add the observer when we set up the shentry, then remove it while we sanitize, then readd it.

Comment 23

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/131a40749cb981542c1e84b570d8d24eff17f399
   https://hg.mozilla.org/integration/mozilla-inbound/rev/4df8820be2aacf5d3cb7a0cc84fdc3acab0015d5
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f98616d79149624aeb956624af1d9714fda2117

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/131a40749cb9
https://hg.mozilla.org/mozilla-central/rev/4df8820be2aa
https://hg.mozilla.org/mozilla-central/rev/7f98616d7914

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to both Beta and ESR60, though I'm not sure what the urgency is here for uplifting given how old the bug is. Is this something we should consider or just let it ride the trains?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Given the limited severity of the bug and the risk of any changes to this code, I don't think we should be uplifting anything here.

Comment 27

[Emil Ghitta, QA [:emilghitta]]

Hi Jesse,

Unfortunately I didn't managed to reproduce this issue by using an affected build (tried on the latest release 61, and with old asan builds).

I tried to reproduce this issue by modifying the prefs Ryan provided in Comment 12, but without luck.

Could you help us by verifying if this issue is fixed in Fx62?.

Thanks a lot!

Comment 28

[Emil Ghitta, QA [:emilghitta]]

It seems that Jesse is unavailable at this time (per abillings). Removing the qe-verify flag for now, as QA cannot reproduce the issue and therefore can't confirm whether it's fixed. 

Perhaps someone else could give it a try?