1433921 - "ASSERTION: Stop called too early or too late" and "ASSERTION: No document in Destroy()!" when closing window in pagehide event after history.back()

Status: RESOLVED DUPLICATE of bug 1432396

(Core :: DOM: Navigation, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: testcase.zip

Discovered in bug 1432396.
there I thought the issue happens only after bug 1193394, but apparently
the issue happens on trunk if I don't use promise.

Steps to reproduce:
  1. Download debug build from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=474d58c9137360c0fa1c85cdd11e3313b33b7cad
  2. Download and extract attached testcase
  3. run Debug build of Firefox
  4. open a.html in testcase (it tries to open popup)
  5. allow popup, and close the opened popup
  6. reload a.html
  7. wait for a minute

Actual result:
  the following assertion failure logged:
> [43483, Main Thread] ###!!! ASSERTION: Stop called too early or too late: 'mDocument', file /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp, line 1831
> [43483, Main Thread] ###!!! ASSERTION: No document in Destroy()!: 'mDocument', file /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp, line 1655

Expected result:
  no assertion failure

Comment 1

[Tooru Fujisawa [:arai]]

the issue is that, already destroyed nsDocumentViewer is set to nsDocShell in nsDocShell::RestoreFromHistory
some analysis is available in bug 1432396 comments.
(not yet sure which one is also true on trunk tho)

Comment 2

[Tooru Fujisawa [:arai]]

To be clear I marked this as security-sensitive just because it's assertion failure.
but I think it was somehow expected, since the code is checking nullness of mDocument property everywhere,
even after the assertion.

Comment 3

[Tooru Fujisawa [:arai]]

So, when window.close is called in pagehide after history.back,
destroyed nsDocumentViewer is set to nsDocShell.mContentViewer

https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/docshell/base/nsDocShell.cpp#8241
> nsresult
> nsDocShell::RestoreFromHistory()
> {
> ...
>   nsCOMPtr<nsIContentViewer> viewer;
>   mLSHE->GetContentViewer(getter_AddRefs(viewer));
> ...
>   FirePageHideNotification(!mSavingOldViewer);
> ...
>   mContentViewer.swap(viewer);

Should we detect the case and do something else?

Comment 4

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

This mostly smells like wrong assertions. Those are some ancient assertions.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Olli Pettay [:smaug] from comment #4)
> This mostly smells like wrong assertions. Those are some ancient assertions.

Thanks :)
I'll verify if there's any place that doesn't check nullness,
and if there is no place, I'll just remove the assertion.

Comment 6

[Tooru Fujisawa [:arai]]

CCing hiro and bz.
bug 1432396 should be fixed once this bug gets fixed.

Comment 7

[Tooru Fujisawa [:arai]]

the patch in bug 1432396 should fix the failure itself.
I'll investigate about the assertions there and file another bug if the assertions need to be removed.