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

VERIFIED FIXED in mozilla1.9.1b3

Status

()

P2
normal
VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: mats, Assigned: roc)

Tracking

({assertion, testcase, verified1.9.1})

Trunk
mozilla1.9.1b3
assertion, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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).
(Reporter)

Comment 1

10 years ago
Created attachment 342822 [details]
stack for the assertion
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?

Comment 3

10 years ago
Created attachment 342950 [details]
simple testcase

Close this testcase to see the assertion.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: nobody → roc
Created attachment 349387 [details] [diff] [review]
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.
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?)
(Reporter)

Comment 8

10 years ago
Created attachment 352131 [details] [diff] [review]
crashtest that asserts (based on "simple testcase")
(Reporter)

Comment 9

10 years ago
Created attachment 352135 [details]
Testcase #2
(Reporter)

Comment 10

10 years ago
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-
(Reporter)

Updated

10 years ago
Whiteboard: [sg:investigate][needs review] → [sg:investigate]
Created attachment 352259 [details] [diff] [review]
fix v2

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]
(Reporter)

Comment 12

10 years ago
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+
(Reporter)

Updated

10 years ago
Whiteboard: [sg:investigate][needs review] → [sg:investigate]
Yes.
Whiteboard: [sg:investigate] → [sg:investigate][needs landing]
Pushed to trunk as 2cbcaca3fcdb
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate][needs landing] → [sg:investigate][needs 191 landing]
Flags: in-testsuite+
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
Keywords: fixed1.9.1 → verified1.9.1
Group: core-security
You need to log in before you can comment on or make changes to this bug.