443655 - "ASSERTION: A frame but no DOM element!?" removing two frames, one of which has an onunload that navigates the other

Status: RESOLVED WORKSFORME

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase (in Mac trunk debug Firefox) triggers:

###!!! ASSERTION: A frame but no DOM element!?: 'requestingElement', file /Users/jruderman/central/mozilla/docshell/base/nsDocShell.cpp, line 6678

This is the same assertion that fired in bug 369126.  I guess the fix in that bug missed this case.

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack for the assertion

(Bug 402982 added this code.)

Removing the iframe elements leads to nsFrameLoader::Destroy() which
calls 'doc->FinalizeFrameLoader(this)' on line 323:
http://hg.mozilla.org/mozilla-central/index.cgi/annotate/951ac1cf93a9/content/base/src/nsFrameLoader.cpp#l285
which in this case delays the Finalize() by appending it to
mFinalizableFrameLoaders on line 4425:
http://hg.mozilla.org/mozilla-central/index.cgi/annotate/951ac1cf93a9/content/base/src/nsDocument.cpp#l4415

In nsDocument::InitializeFinalizeFrameLoaders (stack frame #34) both frame
loaders are in the list, calling Finalize() on the first leads to
nsDocShell::Destroy() which runs FirePageHideNotification() which runs the
script that sets a new location for the second iframe which triggers the
assertion since 'mIsBeingDestroyed' is still false for the docshell
associated with the second iframe.

There is an interval from the time nsFrameLoader::Destroy()
disassociates itself from the docshell until nsDocShell::Destroy()
runs (in this case InitializeFinalizeFrameLoaders() but I think it
can also happen for the nsAsyncDocShellDestroyer() path) where this
assertion can trigger.

FWIW, making an early return if 'requestingElement' is null doesn't work
properly - it leaves the throbber spinning and clicking the Stop button
doesn't make it stop.  The only way I can make it stop is to run
FirePageHideNotification() earlier, from FinalizeFrameLoader(), but
I suspect this is not a solution since FirePageHideNotification()
is synchronous and probably unsafe to do at that point.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to comment #1)
> FWIW, making an early return if 'requestingElement' is null doesn't work
> properly - it leaves the throbber spinning and clicking the Stop button
> doesn't make it stop. 
The same happens if you remove the unload -part from the testcase.
So IMO that is a separate bug, but for this returning early could be ok.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Testcase still insta-crashes on trunk opt builds (and I assume would assert on debug builds).

Comment 4

[Jesse Ruderman]

Attachment: stack for the crash

Interesting, I don't think this crashed when I first reported the bug.

Comment 5

[Jesse Ruderman]

Same symptoms as bug 854864.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This doesn't reproduce anymore.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5960292f2c
Add crashtest. r=me

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6d5960292f2c