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

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?

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.

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?

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?

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

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

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.

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

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.

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.

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

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 on attachment 221112 [details] [diff] [review] With comment added We should take this safety fix on the 1.8.0 branch, in my opinion.

This is what I just landed on trunk.

Fixed on trunk.

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.

Checked in the ESM patch on 1.8 branch.

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

Comment on attachment 221112 [details] [diff] [review] With comment added approved for 1.8.0 branch, a=dveditz for drivers

Checked in the ESM patch for 1.8.0.5

v. fixed 1.5.0.5

Verified with a current Firefox 1.8 branch build.