338122 - Crash [@ nsIMEStateManager::IsActive] when window gets removed on mousedown at button

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes Mozilla within 1 second.
It also crashes Mozilla1.7, so no recent regression.

Comment 1

[Martijn Wargers (dead)]

Attachment: iframe for testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Talkback ID: TB18745342Y
nsIMEStateManager::IsActive   nsIMEStateManager::OnChangeFocus   nsEventStateManager::SetContentState   nsHTMLButtonElement::PostHandleEvent   nsEventTargetChainItem::PostHandleEvent   nsEventTargetChainItem::HandleEventTargetChain   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventDispatcher::Dispatch   nsEventDispatcher::DispatchDOMEvent   nsEventListenerManager::DispatchEvent   nsDOMEventRTTearoff::DispatchEvent   XPTC_InvokeByIndex   XPCWrappedNative::CallMethod

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

Sorry, previous testcase didn't crash, because the iframe can take a while to load.
This testcase uses the data uri. Setting designMode on data uri documents didn't work in older builds, so this testcase doesn't crash in older builds because of that.

Comment 4

[Martijn Wargers (dead)]

Argh! The designMode part isn't even necessary, removing from summary.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Testcase with designMode removed.

Comment 6

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

Attachment: proposed patch

presshell is possibly dead when trying to access the document.

Comment 7

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

I think it would make more sense for the prescontext to keep its own strong reference to nsIDocument. Then we could ensure that GetDocument never returns null.

Comment 8

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

Hmm, would then have to make sure that there isn't any ref-loop.

Comment 9

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

I think any long-lived references are to the presshell, not to the prescontext.

Comment 10

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

Comment on attachment 222183 [details] [diff] [review]
proposed patch

I'll write a patch for nsIDocument member soonish

Comment 11

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

Attachment: Ensure that PresContext has always a document

Like this?
Changed also few PresShell() calls to GetPresShell() to remove useless assertions.

With a quick test I didn't see any new leaks.

There are many cases where presShell->GetDocument() could be changed to presContext->Document(), but that would be a separate clean up bug.

Comment 12

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

Comment on attachment 222494 [details] [diff] [review]
Ensure that PresContext has always a document


> 
> void
> nsEventStateManager::EnsureDocument(nsPresContext* aPresContext)
> {
>   if (!mDocument)
>-    EnsureDocument(aPresContext->PresShell());
>+    EnsureDocument(aPresContext->GetPresShell());
> }

Make that mDocument = aPresContext->Document();

Comment 13

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

Comment on attachment 222494 [details] [diff] [review]
Ensure that PresContext has always a document

+nsIDocument*
+nsPresContext::Document() const
+{
+  NS_ASSERTION(!mShell || !mShell->GetDocument() ||
+               mShell->GetDocument() == mDocument,
+               "nsPresContext doesn't have the same document as nsPresShell!");
+  return mDocument;
+}

Make this inline.

Comment 14

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

Attachment: this was checked in

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with build 2006-05-20-13 of SeaMonkey trunk using the testcase at https://bugzilla.mozilla.org/attachment.cgi?id=222164&action=view under Windows XP; no crash.