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

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Hmm, I have trouble with reproducing the crash here online, but offline I crash relatively easy.

Comment 3

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

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]

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Martijn Wargers (dead)]

Attachment: 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.

Comment 6

[Martijn Wargers (dead)]

Attachment: 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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Martijn, does it help to add:

  nsRefPtr<nsPresContext> presContext = aPresContext;

at the top of nsEventStateManager::PostHandleEvent?

Comment 8

[Martijn Wargers (dead)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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....

Comment 10

[Martijn Wargers (dead)]

(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).

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Martijn Wargers (dead)]

Attachment: patch

Yes, that actually seems to help. I haven't been able to crash in my debug build yet.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Martijn Wargers (dead)]

Attachment: With comment added

I guess like this. This really is not my patch.

Comment 15

[Mats Palmgren (inactive)]

The bug doesn't seem to be reproducable on Linux...

Comment 16

[Mats Palmgren (inactive)]

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.

Comment 17

[Mats Palmgren (inactive)]

Attachment: A different patch

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.

Comment 18

[Martijn Wargers (dead)]

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).

Comment 19

[Mats Palmgren (inactive)]

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).

Comment 20

[Martijn Wargers (dead)]

Yes, your patch seems to fix the crash. I tried 20 times or so in my debug build with the patch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Martijn Wargers (dead)]

(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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Combined patch

This is what I just landed on trunk.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in the ESM patch on 1.8 branch.

Comment 31

[Daniel Veditz [:dveditz]]

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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> "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.

Comment 33

[Daniel Veditz [:dveditz]]

Thanks for the clarification

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 221112 [details] [diff] [review]
With comment added

approved for 1.8.0 branch, a=dveditz for drivers

Comment 35

[Boris Zbarsky [:bzbarsky]]

Checked in the ESM patch for 1.8.0.5

Comment 36

[Kevin Brosnan]

v. fixed 1.5.0.5

Comment 37

[Frank Wein [:mcsmurf]]

Verified with a current Firefox 1.8 branch build.