246712 - Right click "This Frame", but then quickly "View Page Source" causes crash [@ 0x00000ad7 - destroyTimerEvent ] [@ nsCachedChromeChannel::HandleStartLoadEvent ]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608

On that site (which is quite colorful), you can see an orange/brown iframe with
a scrollbar with the big words: "Giel Beelen".
Right click in this iframe, select "This frame".
Wait until the submenu comes up.
Now quickly click on "View Page Source" --> Crash

This happens in Mozilla1.7 RC3.
Talkback ID: TB94608X

It crashes afaik only on this site this way, although in other framed sites you
cannot access the "This Frame" submenu anymore after you followed previous steps.

Reproducible: Always
Steps to Reproduce:
1. See above
2.
3.

Actual Results:  
Crash

Expected Results:  
No crash

Comment 1

[Martijn Wargers (dead)]

Another Talkback ID: TB95152Q

Comment 2

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

0x00690056
destroyTimerEvent 
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 456]
PL_DestroyEvent 
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 713]
PL_HandleEvent 
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 686]
PL_ProcessPendingEvents 
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 612]
nsEventQueueImpl::ProcessPendingEvents 
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsEventQueue.cpp,
line 395]

Does this still happen on a newer build?  Try 2004-06-14-09, if you can.

Comment 3

[Martijn Wargers (dead)]

It still happens with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040614
I don't get any Talkback ID though.

Comment 4

[Martijn Wargers (dead)]

I don't seem to get a crash with a fairly recent debug build of Firefox
(althought the regular one still crashes).
I tried it some twenty times or so.

Comment 5

[Martijn Wargers (dead)]

Also get a crash at http://www.webwereld.nl.
I think this crash happens for all sites with frames.
I believe the timing when clicking on "View page source" is important to get the
crash. I get the feeling I particularly get the crash, when I click on the "View
Page source" just before the submenu tends to close.
Here is another Talkback ID: TB180432X
Using in that case:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/2004-06-26
Firefox/0.9.1

Comment 6

[Adam Hauner]

TB180432 not too usefull:
0x01de8f71
SHELL32.dll + 0x520c24 (0x778b0c24)

Martijn Wargers: Try to delete compreg.dat fine in you Mozilla's components
directory before you start Mozilla. Then please try to reproduce and if TalkBack
appear, send incident and look for TalkBack incident ID.

Comment 7

[Martijn Wargers (dead)]

(In reply to comment #6)
> Martijn Wargers: Try to delete compreg.dat fine in you Mozilla's components
> directory before you start Mozilla. Then please try to reproduce and if TalkBack
> appear, send incident and look for TalkBack incident ID.

Ok, done that. I've done three crashes, hth: TB227361K, TB227365M and TB227369W.
Using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040629
Firefox/0.8.0+

Comment 8

[Adam Hauner]

Martijn: Due to a recent Talkback database upgrade aren't older incidents
available. Could I ask you again for TalkBack incident ID? I do apologize for
complication.

Comment 9

[Martijn Wargers (dead)]

Ok, here are three new TB's: TB320459G, TB320495Q and TB320502Y.

Comment 10

[Adam Hauner]

TB320495Q and TB320502Y look like:
0x00000ad7
destroyTimerEvent 
[c:/builds/tinderbox/firefox-0.9.1/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 456]
SETUPAPI.DLL + 0x30c24 (0x778b0c24)

TB320459G:
nsCachedChromeChannel::HandleStartLoadEvent 
[c:/builds/tinderbox/firefox-0.9.1/WINNT_5.0_Clobber/mozilla/chrome/src/nsChromeProtocolHandler.cpp,
line 447]
SETUPAPI.DLL + 0x30c24 (0x778b0c24)

Comment 11

[aaronr]

please try again.  I could not recreate on 1.7.1, though if I tried this
multiple times eventually the "this frame" popup menu stopped appearing.  But no
crash.

Comment 12

[Martijn Wargers (dead)]

Three new crashes: TB499344E,TB499325Z and TB499307X. Using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.1) Gecko/20040707
I admit, sometimes it can be very hard to get the crash. I have to repeat the
steps 10 times sometimes before I get the crash. Also, like you said, the "this
frame" popup menu sometimes stops working when trying to trigger the crash.

Comment 13

[Adam Hauner]

All new stacks look same:
0x00690056
xpcom.dll + 0x29025 (0x61d49025)
xpcom.dll + 0x29957 (0x61d49957)
xpcom.dll + 0x29928 (0x61d49928)
xpcom.dll + 0x2986b (0x61d4986b)
xpcom.dll + 0x27ad1 (0x61d47ad1)

Comment 14

[Martijn Wargers (dead)]

I have a better way to get the crash:
- Go to gmail (you've to be logged in and I have the setting 100 conversations
per page)
- Right Click somewhere in the page
- Hover over "This frame", wait until the submenu comes up.
- Now quickly choose "Select All".

Crashes quite reliably for me (50% of the cases) with 2005-09-04 trunk build.
Talkback ID's: TB9077106M TB9077132E

Comment 15

[Rotem Liss]

I think it is a duplicate of Bug 268399: it is exactly the problem described in the bug.

Comment 16

[chris hofmann]

anyone still see the problems in comment 0 and/or comment 14?  I was unable to reproduce using
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060313 Firefox/1.5.0.2

Comment 17

[Martijn Wargers (dead)]

I can still reproduce, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060311 Firefox/1.6a1

Comment 18

[Boris Zbarsky [:bzbarsky]]

The problem is that nsMenuPopupFrame uses itself as a timer callback and doesn't cancel the timer in Destroy().  That's wrong.  Of course just using itself as a timer callback is wrong -- see bug 241733.

Martijn, do you want to take a stab at fixing this based on the patch in bug 241733?  If not, that's fine; I'll hunt someone down.  ;)

Changing deps to make this block the other similar-looking crashes.

Comment 19

[Martijn Wargers (dead)]

Attachment: patch

I really have no idea what I'm doing here, I more or less copied it from bug 241733 and then fixed all the compiler errors I got.
But it seems to fix the issue, I think.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215050 [details] [diff] [review]
patch

>Index: layout/xul/base/src/nsMenuPopupFrame.h
>+ * nsMenuPoupFrame, so that it will exist as long as the timer holds a reference

nsMenuPopupFrame

>+ * to it. The callback is delegated to the contained nsMenuFrame as long as

And here.

Also, I still think we should cancel that timer in Destroy() in addition to this patch (if it's non-null, etc).

r+sr=bzbarsky with that.

Thanks for doing this!

Comment 21

[Martijn Wargers (dead)]

Attachment: updated patch, fixed comments

So I guess this is ready for check-in then.

Comment 22

[Martijn Wargers (dead)]

Oh wait, I should "cancel that timer in Destroy() in addition to this patch (if it's non-null, etc)".

Comment 23

[Martijn Wargers (dead)]

Attachment: patch

With:
 nsMenuPopupFrame::Destroy(nsPresContext* aPresContext)
 {
+  if (mCloseTimer)
+    mCloseTimer->Cancel();

Boris, you meant this, right?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pretty much; I'd call ClearFrame() before calling Cancel(), but other than that looks great.

Comment 25

[Martijn Wargers (dead)]

Attachment: patch

Ok, calling ClearFrame() before calling Cancel() now.
This one is ready for check-in.

Comment 26

[Martijn Wargers (dead)]

Checking in layout/xul/base/src/nsMenuPopupFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsMenuPopupFrame.h,v  <--  nsMenuPopupFrame
.h
new revision: 1.81; previous revision: 1.80
done
Checking in layout/xul/base/src/nsMenuPopupFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp,v  <--  nsMenuPopupFra
me.cpp
new revision: 1.277; previous revision: 1.276
done

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few notes, at least for the future:

Please wrap comments at 78 characters or less (I use 72).  Otherwise we'll all keep making our windows a little wider, and a little wider, and a little wider...  80 is a pretty common standard.

~nsMenuPopupTimerMediator should really be private, since it's non-virtual (and should be protected if it were).

It might have made sense to make the constructor and destructor inline; they're simple enough.

Comment 28

[Alex Sirota (iosart)]

*** Bug 340045 has been marked as a duplicate of this bug. ***

Comment 29

[Alex Sirota (iosart)]

I can confirm that this solves the problem, after pretty aggressive testing...

Any chance of this getting merged into the branch?

I'm getting these crashes with the latest Bon Echo 3 - it would be great if this could be solved in Firefox 2.0

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Martijn, is landing this on the branch ok with you?  Normally the patch author should request approval -- on the principle that two people need to agree, both the patch author and the approver.

Also, which patch is it that should go in, if so?

Comment 31

[Martijn Wargers (dead)]

(In reply to comment #30)
> Martijn, is landing this on the branch ok with you? 

Yes, sure. It was on my to-do list, actually. But I was doubting a little, because of the suggestions you made in comment 27. I know those are merely suggestions to make the patch better, but I'm afraid I can't fulfill those suggestions, because I still don't understand how to do those.

> Also, which patch is it that should go in, if so?

The last one I posted.

Comment 32

[Martijn Wargers (dead)]

Attachment: patch for 1.8.1 branch

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 215050 [details] [diff] [review]
patch

plussed approval request on correct patch; clearing this one

Comment 34

[Martijn Wargers (dead)]

Attachment: real 1.8.1 patch ready for check-in

Comment 35

[Martijn Wargers (dead)]

Checking in nsMenuPopupFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp,v  <--  nsMenuPopupFra
me.cpp
new revision: 1.259.2.5; previous revision: 1.259.2.4
done
Checking in nsMenuPopupFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsMenuPopupFrame.h,v  <--  nsMenuPopupFrame
.h
new revision: 1.70.24.2; previous revision: 1.70.24.1
done

Checked into 1.8.1 branch with help from dbaron.

Comment 36

[Jeff F.]

This bug has returned, and should be reopened TB20252976M , TB20252961W , TB20252955Y