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

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
13 years ago
2 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)

Reporter

Description

13 years ago
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
Reporter

Comment 1

13 years ago
Posted file testcase
Reporter

Comment 2

13 years ago
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?
Reporter

Comment 5

13 years ago
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.
Reporter

Comment 6

13 years ago
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?
Reporter

Comment 8

13 years ago
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....
Reporter

Comment 10

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

Comment 12

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

Comment 14

13 years ago
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)
Assignee

Comment 15

13 years ago
The bug doesn't seem to be reproducable on Linux...
Keywords: pp
Assignee

Comment 16

13 years ago
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.
Assignee

Comment 17

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

Comment 18

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

Updated

13 years ago
Attachment #221112 - Flags: superreview?(roc)
Attachment #221112 - Flags: review?(roc)
Attachment #221112 - Flags: approval-branch-1.8.1?(roc)
Assignee

Comment 19

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

Comment 20

13 years ago
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+
Reporter

Comment 23

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

Updated

13 years ago
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
Last Resolved: 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

Comment 36

13 years ago
v. fixed 1.5.0.5
Verified with a current Firefox 1.8 branch build.
Reporter

Updated

13 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.