Crash when window gets destroyed when clicking on iframe which has focus event handler

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
13 years ago
4 months ago

People

(Reporter: martijn.martijn, Assigned: mats)

Tracking

(6 keywords)

Dependency tree / graph
Bug Flags:
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

See upcoming testcase, which crashes current trunk Mozilla builds when clicking in the iframe.
The crash doesn't happen always. Then reload the document and try again. Normally, the crash should happen withing 5 times trying.

I think this regressed between 2006-01-25 and 2006-01-26:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-01-25+07&maxdate=2006-01-26+11&cvsroot=%2Fcvsroot

Maybe a regression from bug 281139?

Talkback ID:
MSVCR80.dll + 0x150aa (0x602250aa)
nsVoidArray::RemoveElementsAt   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent
Posted file testcase
Hmm, I have trouble with reproducing the crash here online, but offline I crash relatively easy.
I can't reproduce the crash, but I do see the following:
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 587
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 587
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "parent has no properties" {file: "file:///home/smaug/mozilla/tests/336582_crash_focus_files/a.html" line: 6}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
smaug, what OS are you testing on?  Given the stack snippet and the fact that this is focus code, this has a nontrivial chance of being Windows-only.... :(

Martijn, do you want to try a local backout to see whether bug 281139 is indeed the issue here?
Blocks: 281139
Flags: blocking1.9a1?
Posted file testcase2
Yes, I'll test whether a backout of bug 281139 helps.
I'll test it with this testcase, because the first testcase doesn't want to crash in my debug build (I guess because of the assertions).
The assertions that are happening with the first testcase could be a different issue, because with this testcase I crash with the same backtrace, but I don't get the assertions.
Posted file backtrace
Ok, I still crash in my debug build after I've backed out the patch from bug 281139.
This is the backtrace I get from the debug build.

I rechecked the regression range I mentioned in comment 0, and I still see that same regression range. Maybe this is a regression from the frame display lists patch, bug 317375.
Martijn, does it help to add:

  nsRefPtr<nsPresContext> presContext = aPresContext;

at the top of nsEventStateManager::PostHandleEvent?
No, that doesn't seem to help.
This is what I had in my tree:
http://wargers.org/mozilla/bug336582/esm.patch
This is the backtrace I get with that:
http://wargers.org/mozilla/bug336582/bt.txt
Um... Why that first change to make the presshell a weak pointer?

Note that it looks like we now _do_ get through nsEventStateManager::PostHandleEvent fine...

I _am_ a little confused by the stack.  The only ways setting mCurrentEventContent there can crash are that the value is already bogus somehow or the presshell is deleted (with similar consequences).  But the view manager holds a ref to the presshell when it calls HandleEvent....
(In reply to comment #9)
> Um... Why that first change to make the presshell a weak pointer?
That's the backout of the patch from bug 281139 (as mentioned in comment 6).
Ah.  So what if you undo that backout _and_ make the change I suggested in comment 7?  I'd guess you still crash, but just to check?
No longer blocks: 281139
Posted patch patch (obsolete) — Splinter Review
Yes, that actually seems to help. I haven't been able to crash in my debug build yet.
Comment on attachment 221107 [details] [diff] [review]
patch

Might want to make a comment about keeping the prescontext alive because we need it after event dispatch or something, I guess.  I really shouldn't be reviewing this, though.  roc, would you do the honors?
Attachment #221107 - Flags: superreview?(roc)
Attachment #221107 - Flags: review?(roc)
Attachment #221107 - Flags: approval-branch-1.8.1?(roc)
I guess like this. This really is not my patch.
Attachment #221107 - Attachment is obsolete: true
Attachment #221112 - Flags: superreview?(roc)
Attachment #221112 - Flags: review?(roc)
Attachment #221112 - Flags: approval-branch-1.8.1?(roc)
Attachment #221107 - Flags: superreview?(roc)
Attachment #221107 - Flags: review?(roc)
Attachment #221107 - Flags: approval-branch-1.8.1?(roc)
The bug doesn't seem to be reproducable on Linux...
Keywords: pp
Comment on attachment 221112 [details] [diff] [review]
With comment added

I still crash both testcases with this patch (debug Firefox WinXP),
in PresShell::PopCurrentEventInfo() with mCurrentEventFrameStack=0xdddddddd
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=3.912&root=/cvsroot&mark=5669#5662
called at the end of HandlePositionedEvent(), the shell is destroyed
at this point.
As Boris points out in comment 9, the view manager holds a strong ref
to the shell during event handling, the problem is that this shell
decides to let a different shell handle the event.
Attachment #221168 - Flags: review?(bzbarsky)
Hmm, yes, now I too can crash with my debug build with the crash (I thought I couldn't previously, that's the problem with testcase that don't crash 100% of the time).
Attachment #221112 - Flags: superreview?(roc)
Attachment #221112 - Flags: review?(roc)
Attachment #221112 - Flags: approval-branch-1.8.1?(roc)
Martijn, feel free to test my patch as well, since we seem to have slightly
different behaviour on the same OS (I always get a crash on the first click
with both testcases).
Yes, your patch seems to fix the crash. I tried 20 times or so in my debug build with the patch.
Comment on attachment 221112 [details] [diff] [review]
With comment added

So wait.  We still want the ESM change, don't we?  That's needed whether we take Mats change or not, as far as I can tell.
Attachment #221112 - Flags: superreview?(roc)
Attachment #221112 - Flags: review?(roc)
Attachment #221112 - Flags: approval-branch-1.8.1?(roc)
Comment on attachment 221168 [details] [diff] [review]
A different patch

Make this an nsCOMPtr, and r+sr=bzbarsky.  Please land on 1.8 branch too.
Attachment #221168 - Flags: superreview+
Attachment #221168 - Flags: review?(bzbarsky)
Attachment #221168 - Flags: review+
Attachment #221168 - Flags: approval-branch-1.8.1+
(In reply to comment #21)
> (From update of attachment 221112 [details] [diff] [review] [edit])
> So wait.  We still want the ESM change, don't we?  That's needed whether we
> take Mats change or not, as far as I can tell.
Well, I didn't crash anymore with only Mats'patch in my tree.
Yeah, but the other patch is clearly needed on general grounds.  We could try to create a testcase to prove that, but it doesn't seem worth it.
Attachment #221112 - Flags: superreview?(roc)
Attachment #221112 - Flags: superreview+
Attachment #221112 - Flags: review?(roc)
Attachment #221112 - Flags: review+
Attachment #221112 - Flags: approval-branch-1.8.1?(roc)
Attachment #221112 - Flags: approval-branch-1.8.1+
Blocks: 337219
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Comment on attachment 221168 [details] [diff] [review]
A different patch

We should take this safety fix on the 1.8.0 branch, in my opinion.
Attachment #221168 - Flags: approval1.8.0.5?
Comment on attachment 221112 [details] [diff] [review]
With comment added

We should take this safety fix on the 1.8.0 branch, in my opinion.
Attachment #221112 - Flags: approval1.8.0.5?
This is what I just landed on trunk.
Assignee: events → mats.palmgren
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 221168 [details] [diff] [review]
A different patch

So actually, this is not a problem on the branches, since this code was only added on trunk as part of the frame display lists patch.
Attachment #221168 - Flags: approval1.8.0.5?
Attachment #221168 - Flags: approval-branch-1.8.1+
Checked in the ESM patch on 1.8 branch.
Keywords: fixed1.8.1
The 1.8.0.5 triage team is confused between approval requests, "not a problem on branches", and "checked into 1.8 branch". Are the approval requests on the right patches and do you still want it on the 1.8.0 branch?
> "not a problem on branches", and "checked into 1.8 branch"

There are two patches in this bug.  Both are needed on trunk, imo.  The two sentences you quote apply to the two different patches.

> Are the approval requests on the right patches

Yes.

> do you still want it on the 1.8.0 branch?

Yes.
Thanks for the clarification
Flags: blocking1.8.0.5? → blocking1.8.0.5+
Comment on attachment 221112 [details] [diff] [review]
With comment added

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #221112 - Flags: approval1.8.0.5? → approval1.8.0.5+
Checked in the ESM patch for 1.8.0.5
Keywords: fixed1.8.0.5
v. fixed 1.5.0.5
Verified with a current Firefox 1.8 branch build.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.