Closed Bug 459613 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: MatsPalmgren_bugz, Assigned: roc)

References

Details

(Keywords: assertion, testcase, verified1.9.1, Whiteboard: [sg:investigate])

Attachments

(5 files, 1 obsolete file)

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).
Whiteboard: [sg:investigate]
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.
Flags: blocking1.9.1?
Attached file simple testcase
Close this testcase to see the assertion.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #349387 - Flags: superreview?(mats.palmgren)
Attachment #349387 - Flags: review?(mats.palmgren)
Whiteboard: [sg:investigate] → [sg:investigate][needs review]
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?
OS: Linux → All
Hardware: PC → All
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...
(And you can of course use class="reftest-wait" for a crashtest.  Does it assert if you create and destroy an iframe?)
Attached file Testcase #2
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.
Attachment #349387 - Flags: superreview?(mats.palmgren)
Attachment #349387 - Flags: review?(mats.palmgren)
Attachment #349387 - Flags: review-
Whiteboard: [sg:investigate][needs review] → [sg:investigate]
Attached patch fix v2Splinter Review
Fix that by propagating information about frame teardown through nsEditor::PreDestroy. Also rolls up the tests.
Attachment #349387 - Attachment is obsolete: true
Attachment #352259 - Flags: superreview?(mats.palmgren)
Attachment #352259 - Flags: review?(mats.palmgren)
Whiteboard: [sg:investigate] → [sg:investigate][needs review]
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?
Attachment #352259 - Flags: superreview?(mats.palmgren)
Attachment #352259 - Flags: superreview+
Attachment #352259 - Flags: review?(mats.palmgren)
Attachment #352259 - Flags: review+
Whiteboard: [sg:investigate][needs review] → [sg:investigate]
Yes.
Whiteboard: [sg:investigate] → [sg:investigate][needs landing]
Pushed to trunk as 2cbcaca3fcdb
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate][needs landing] → [sg:investigate][needs 191 landing]
Flags: in-testsuite+
Blocks: 463681
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.)
Blocks: 469606
It would be helpful to land this on branch soon, as it will simplify life in Thunderbird-land.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a49288810f16
Keywords: fixed1.9.1
Whiteboard: [sg:investigate][needs 191 landing] → [sg:investigate]
Target Milestone: --- → mozilla1.9.1b3
Duplicate of this bug: 345618
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
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.