373344 - Mousedown event listener changing body style and alert()ing crashes [@ PresShell::HandleEventInternal] browser

Status: VERIFIED FIXED

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

Description

[tgirmann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

Prerequisites:
* Body has position:relative and height:100%
* A mousedown event listener is added to window, which sets body position to static and then displays a message box. After closing the message box the browser crashes

Reproducible: Always

Steps to Reproduce:
Load the example, click anywhere, click OK.
Actual Results:  
Browser crashes.

Expected Results:  
Browser shouldn't crash.

This crash occurs probably due to memory corruption or a freed object being accessed (a non existing object's function list is dereferenced and jumped to - this ends in various places).

I'm using the following extensions:
* firebug
* noscript
* greasemonkey
* scrapbook
* web developer
(and a few others which don't seem to be relevant here)

This *might* be a security problem but I cannot make guarantees about that right now.

about:buildconfig

Build platform
target
i586-pc-msvc

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	12.00.8804 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	12.00.8804 	-TP -nologo -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=release --enable-official-branding --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-svg --enable-canvas --enable-update-packaging

Comment 1

[tgirmann]

Attachment: Html file that crashes your browser (don't click it if you don't want the browser to crash)

Html file that crashes your browser (don't click it if you don't want the browser to crash)

Comment 2

[Martijn Wargers (dead)]

Talkback ID: TB30077687Z
0x02aa7011
PresShell::HandleEventInternal  [mozilla/layout/base/nsPresShell.cpp, line 6497]
PresShell::HandleEvent  [mozilla/layout/base/nsPresShell.cpp, line 6261]
nsViewManager::HandleEvent  [mozilla/view/src/nsViewManager.cpp, line 2559]
nsViewManager::DispatchEvent  [mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1389]
nsWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6442]
ChildWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6689]
nsWindow::WindowProc  [mozilla/widget/src/windows/nsWindow.cpp, line 1577]
USER32.dll + 0x8709 (0x77d18709)
USER32.dll + 0x87eb (0x77d187eb)
USER32.dll + 0x89a5 (0x77d189a5)
USER32.dll + 0x89e8 (0x77d189e8)
nsAppShell::Run  [mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 152]
main  [mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)

It does crash for me on the latest 1.8.1 branch build, but not the latest trunk build.

Comment 3

[Martijn Wargers (dead)]

This seems to have been fixed somehow between 2005-12-01 and 2005-12-02:
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=2005-12-01+04&maxdate=2005-12-02+09&cvsroot=%2Fcvsroot
Fixed by bug 312566, perhaps?

Comment 4

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

Doesn't crash in Linux (branch/trunk)

Comment 5

[Window Snyder]

If you are not the right person to assign this to, please help us find someone that is.

Comment 6

[Johnny Stenback (:jst)]

This is indeed fixed by landing the fix for bug 312566 on the 1.8 branch. I'll attach a branch version of the patch, and nominate it for 1.8.

Comment 7

[Daniel Veditz [:dveditz]]

At the crash site the aView passed into nsEventStateManager::PostHandleEvent looks semi trashed, or at least its mViewManager is.

Fixed for trunk, made a blocker for the next branch releases.

Comment 8

[Ere Maijala (slow)]

Unfortunately the branch patch I created for bug 312566 doesn't seem to fix this :(

Comment 9

[Ere Maijala (slow)]

Looks like this is somewhat timing-dependent. When I had some strategically placed breakpoints and ran it slowly in the debugger, it didn't crash, but without it crashes regardless of the patch for bug 312566. 

I suspect the fact that patch seemed to cure it in trunk is just good luck and it should be investigated more. I can see if I can find out anything in branch.

Comment 10

[Daniel Veditz [:dveditz]]

Ere: were you able to find out anything about the branch crash?

Comment 11

[Daniel Veditz [:dveditz]]

Will have to wait for next release since we don't even have a start.

Comment 12

[Ere Maijala (slow)]

The thrashed view is destroyed right after the view manager has created the event target list containing this view. It's timing-dependent and sometimes it seems the view gets destroyed before the list is built avoiding the crash. I need to do some more digging. Anyway, I'm reopening this because I'm quite sure the fix for bug 312566 just masked the problem in trunk.

Comment 13

[Ere Maijala (slow)]

Now it seems a spurious mouse down event is generated for the destroyed view. Don't know yet where it comes from.

Comment 14

[Ere Maijala (slow)]

Actually, it's so that when the initial NS_MOUSE_LEFT_BUTTON_DOWN is being processed, the processing is blocked when the popup is displayed. When the button is released, NS_MOUSE_LEFT_BUTTON_UP is handled completely, but the DOWN event is still blocked. Finally when the popup is dismissed the DOWN event handling proceeds to nsEventStateManager::PostHandleEvent, finds out that mouseup was already processed and "state is out of whack". It then tries to reset mouse capture using the original view that doesn't exist anymore. I'll do yet some more digging but feel free to chime in with any suggestions on how to fix this.

Comment 15

[Daniel Veditz [:dveditz]]

Will wait for trunk progress before blocking on the branch

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Olli, can you take a look at this?

Comment 17

[Martijn Wargers (dead)]

This bug should not be blocking1.9 since it's not a problem on trunk.

Comment 18

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

I can't really take this because of comment #4.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #17)
> This bug should not be blocking1.9 since it's not a problem on trunk.

According to comment 12 it is.

Ere, got any updates here? Assigning this to you since you seem to have the best handle on it. Let me know if you need help with anything.

Comment 20

[Ere Maijala (slow)]

Uhh.. I'll do my best. I can think of three ways to fix this:

1. Keep the view alive until after PostHandleEvent is done
2. Let PostHandleEvent know that the view is gone
3. Pass viewmanager to PostHandleEvent so it doesn't have to get it from view

Not sure yet if any of these is viable. I don't know how to do 1 or 2, but perhaps 3 would work.

Comment 21

[Ere Maijala (slow)]

Attachment: Possible patch

Would this be acceptable? This would also work in securing trunk.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 281504 [details] [diff] [review]
Possible patch

Smaug should review this. Looks reasonable to me, but I don't necessarily know all the details around this. I'll sr once Smaug's had a chance to look.

Comment 23

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

Comment on attachment 281504 [details] [diff] [review]
Possible patch


>+// 0a6feb7b-e45e-43e4-9120-510fa50ecdb6
> #define NS_IEVENTSTATEMANAGER_IID \
>-{ 0x2270e188, 0x6743, 0x441e, \
>-  { 0xb6, 0xe1, 0xaf, 0x83, 0xf1, 0x04, 0x7a, 0x53 } }
>-
>+{ 0x0a6feb7b, 0xe45e, 0x43e4, \
>+  { 0x91, 0x20, 0x51, 0x0f, 0xa5, 0x0e, 0xcd, 0xb6 } }

The patch is for 1.8 branch, right? You can't change interfaces there. 
You have to create nsIEventStateManager_18 interface, I think, and put the 
new PreHandleEvent/PostHandleEvent methods there. Though, if this bug happens on trunk too, 
it is better to make the trunk patch first.

>+      if (aEvent->message == NS_MOUSE_LEFT_BUTTON_DOWN) 
>+        printf("** LEFT BUTTON DOWN\n");
I suppose you weren't going to leave this here.

>+    nsIViewManager* viewManager = aView ? aView->GetViewManager() : nsnull;
>+
>     // 1. Give event to event manager for pre event state changes and
>     //    generation of synthetic events.
>     rv = manager->PreHandleEvent(mPresContext, aEvent, mCurrentEventFrame,
>-                                 aStatus, aView);
>+                                 aStatus, aView, viewManager);
> 
>     // 2. Give event to the DOM for third party and JS use.
>     if ((GetCurrentEventFrame()) && NS_SUCCEEDED(rv)) {
>@@ -6501,7 +6503,7 @@
>       if (NS_SUCCEEDED (rv) &&
>           (GetCurrentEventFrame() || !NS_EVENT_NEEDS_FRAME(aEvent))) {
>         rv = manager->PostHandleEvent(mPresContext, aEvent, mCurrentEventFrame,
>-                                      aStatus, aView);
>+                                      aStatus, aView, viewManager);
>       }
>     }
>   }

This doesn't look safe. What is keeping viewManager alive?

Would it be possible to have the is-view-alive-check in presshell, then you wouldn't need to
change nsIEventStateManager?

Could you please use -p when creating patches.

Comment 24

[Ere Maijala (slow)]

I've no idea what's keeping viewManager alive, it just does stay there long enough. I'd love the is-view-alive-check, but I don't know how to do it. 

Comment 25

[Johnny Stenback (:jst)]

Smaug, any way you could help out here and help Ere come up with alternate approaches to this view ownership problem?

Comment 26

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

Attachment: for testing

Ere, could you test this patch? The patch is for trunk, but should be
easily ported to branch too. Does it fix the crash?
(I need to still ensure that the patch does the right thing in all cases, so 
basically ask roc about views)

Comment 27

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

Or anyone willing to test this on Windows?

Comment 28

[Ere Maijala (slow)]

Comment on attachment 284940 [details] [diff] [review]
for testing

Had to apply manually to branch, but yes, it works.

Comment 29

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

Applies to branch using --fuzz=7

Comment 30

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

Comment on attachment 284940 [details] [diff] [review]
for testing

Roc, what do you say about this?
(Branch patch should use NS_STATIC_CAST(), not static_cast<>)

Comment 31

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

Comment on attachment 284940 [details] [diff] [review]
for testing

I think we want this on trunk first.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 284940 [details] [diff] [review]
for testing

a=endgame drivers for after M9 freeze

Comment 33

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

Checked in, but this caused
###!!! ASSERTION: View doesn't have a frame.: '!aView || viewownerframe.GetFrame()
on bm-xserve11

Roc, are there some special cases in view<->frame ownership?

Comment 34

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

Though, I think I have a better solution already. Need to test it first.

Comment 35

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

Um, or that would mean creating nsWeakView :(
Would be better to find some other solution, so basically fix the patch, I hope.

Comment 36

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

Attachment: nsWeakView

Roc, what do you think about adding weakviews? I'd rather fix the previous
patch, but I'm not sure how.
I guess I'll try to reproduce the "!aView || viewownerframe.GetFrame()"
problem and see which view doesn't have frame (should have done that even before
creating the weakview patch).

Comment 37

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

(In reply to comment #36)
> I guess I'll try to reproduce the "!aView || viewownerframe.GetFrame()"
> problem and see which view doesn't have frame.
Haven't found the way to reproduce.. 

Comment 38

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

Ere, if you had the chance to test the patch ;)

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsScrollPortViews don't have frames, maybe they're hitting this? Maybe you should bump the view up to the nearest ancestor that has a frame.

Comment 40

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

Attachment: v2, no WeakViews

This handles root view in a special way. With this patch I can't see the
assertion. Note, the rootview thing happens very rarely, as far as see only
when opening a new tab which causes NS_GOT/LOSTFOCUS to be dispatch to rootview.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not just set viewOwnerFrame to GetClientData and hold a strong reference to the view manager there? Then later if viewOwnerFrame is null or dead, extract the view manager's root view and use it. I don't see the need to get the root view at the beginning.

Comment 42

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

I'm at least trying to prevent the warning when possible:
NS_WARN_IF_FALSE(!safeView || safeView == aView, "View changed while dispatching an event!");
If I'd use root view always when aView gets deleted, the view would certainly
be different.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does that matter?

Comment 44

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

Currently nothing in ESM::PostHandle cares about that. 

Comment 45

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

Hmm, but I'd like to keep that assertion that noticed the problem in the first
patch. For that I need to set rootvm only it root view is used or add
#ifdef DEBUG part to test it.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

if you can stick the complexity behind #ifdef DEBUG, that's helpful.

Comment 47

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

Attachment: v3

Comment 48

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

bm-xserve11 still gave me an assertion. ugh.
Next thing is to check scrollableviews, I think.

Comment 49

[Rob Campbell [:rc] (:robcee)]

is bug 403781 a dupe of this?

Comment 50

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

Roc, would having nsWeakViews be really bad? That should keep the code
in PresShell very simple.
The patch has a typo s/NS_VIEW_DISOWN_WIDGET/NS_VIEW_DISOWNS_WIDGET/

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, let's do nsWeakView :-(

Comment 52

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

I'll ask someone (ere) who sees the crash to test it first.

Comment 53

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

Attachment: nsWeakView2

Comment 54

[Ere Maijala (slow)]

Comment on attachment 288716 [details] [diff] [review]
nsWeakView2

It was a pain to apply to the branch, but it seems to work.

Comment 55

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

Comment on attachment 288716 [details] [diff] [review]
nsWeakView2

So, trying weakview...

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So if I create two nsWeakViews for the same view and destroy the first one before a second one, we have a problem right?

Comment 57

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

nsWeakViews, as implemented in the patch, are really just for the stack. Never-ever use them in heap.

Comment 58

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

I could add a comment that nsWeakViews must be used only as stack objects.
I don't think there are many use cases to support heap (there are use cases to support nsWeakFrame in heap).

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'd better add that comment. Can you overload operator new so that attempts to heap-allocate nsWeakViews fail? Can you add assertions that fire if we attempt to destroy nsWeakViews out of order?

Comment 60

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

Attachment: +assertion and comment

Comment 61

[tgirmann]

Attachment: Another test case

This is another test case that crashes the browser. Can someone please review whether this is the same bug? I'm not 100% sure - there are similarities (changing display property in event handler) but also some differences (involving Midas).

Comment 62

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

Comment on attachment 290519 [details]
Another test case

Doesn't seem to crash on trunk and on branch the stack trace looks quite different to this bug.
But the trace does look familiar. Could you perhaps file a new (security sensitive) bug and attach the testcase there.

Comment 63

[Martijn Wargers (dead)]

The issue in comment 61 was filed as bug 405783.

Comment 64

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

Attachment: for 1.8

Anyone (who can reproduce the crash) willing to test this on 1.8?

Comment 65

[Ere Maijala (slow)]

Comment on attachment 290723 [details] [diff] [review]
for 1.8

Works great and easy to apply :)

Comment 66

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

Comment on attachment 290723 [details] [diff] [review]
for 1.8

Great. Then asking approval

Comment 67

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

Comment on attachment 290723 [details] [diff] [review]
for 1.8

or perhaps first review

Comment 68

[Daniel Veditz [:dveditz]]

Comment on attachment 290723 [details] [diff] [review]
for 1.8

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 69

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008010104 Minefield/3.0b3pre ID:2008010104 and the testcases from this bug. No Crash on testcases - changing bug to verified

Comment 70

[Al Billings [:abillings - ex-MoCo]]

Verified in branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre. No crash with repeated use of test case, unlike 2.0.0.11 testing.

Comment 71

[Alexander Sack]

distro patches block 1.8.0.15.

Comment 72

[Alexander Sack]

Comment on attachment 290723 [details] [diff] [review]
for 1.8

this patch ships unmodified in distros. caillon, please sign off.

Comment 73

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 290723 [details] [diff] [review]
for 1.8

a=caillon for 1.8.0.15

Comment 74

[Christopher Aillon (sabbatical, not receiving bugmail)]

Committed to the 1.8.0 branch