459613 - ASSERTION: GetPrimaryFrameFor() called while nsFrameManager is being destroyed!

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. load the mochitest content/base/test/test_bug398243.html
2. reload

###!!! ASSERTION: GetPrimaryFrameFor() called while nsFrameManager is being destroyed!: 'Error', file /usr/moz/hg1/layout/base/nsFrameManager.cpp, line 332

Filing as security-sensitive because GetPrimaryFrameFor() during frame
destruction can potentially lead to nasty crashes (using freed frames).

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack for the assertion

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, so we either need someone here (selection code?) to check the frame manager state or do some part of this cleanup async...  Or remove the assert and just make the frame manager return null in this case.

Doesn't look like a form controls issue much, since the form control code is just doing the only thing editor will let it do.

Comment 3

[Jesse Ruderman]

Attachment: simple testcase

Close this testcase to see the assertion.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

1) tell spellchecker Cleanup() if the editor is going away. If it is, then we don't need to bother removing spellcheck ranges since all that content is going away. That avoids us doing dangerous work during frame destruction.

2) Fix presshell so that if something causes presshell destruction we don't keep going through the event phases so we don't do frame lookups on a dying frame manager and trigger the GetPrimaryFrameFor assertion.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure how to test this. Is a mochitest that does window.open of attachment #342950 [details] and immediately closes it the best way?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What we're doing for assertions now is putting them in crashtests (which are written like reftests), although we're not actually running them under automation that detects assertions...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And you can of course use class="reftest-wait" for a crashtest.  Does it assert if you create and destroy an iframe?)

Comment 8

[Mats Palmgren (inactive)]

Attachment: crashtest that asserts (based on "simple testcase")

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 349387 [details] [diff] [review]
fix

There's a misspell-indication after loading Testcase #2 with this patch.
r- based on that.

Nit:
> layout/base/nsPresShell.cpp
>+    if (!mIsDestroying && (GetCurrentEventFrame()) && NS_SUCCEEDED(rv)) {

FYI, GetCurrentEventFrame() now returns null when mIsDestroying is true -
you can have the explicit test there for clarity if you want though.

Remove the extra parentheses around the call.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Fix that by propagating information about frame teardown through nsEditor::PreDestroy. Also rolls up the tests.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 352259 [details] [diff] [review]
fix v2

r+sr=mats with the following nit:

> editor/txtsvc/public/nsIInlineSpellChecker.idl
>+  [noscript] void cleanup(in boolean aDestroyingEditor);

> extensions/spellcheck/src/mozInlineSpellChecker.cpp
>+nsresult mozInlineSpellChecker::Cleanup(PRBool aDestroyingEditor)
> ...
>+    if (!aDestroyingEditor) {

These should also be renamed aDestroyingFrames?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed to trunk as 2cbcaca3fcdb

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

When you land this on 1.9.1, could you land bug 463681 as well?  (It's worth double-checking the logs for the leak test tinderboxes for ASSERTIONs first, though.)

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

It would be helpful to land this on branch soon, as it will simplify life in Thunderbird-land.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a49288810f16

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED (on both attached testcases as well as no results for a red on the attached crashtest via bugzilla) using debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028