675614 - onclick alert browser hang

Status: RESOLVED INCOMPLETE

(Core :: DOM: Selection, defect)

Description

[Stefan]

User Agent:  

Steps to reproduce:

1. Open https://bugzilla.mozilla.org/attachment.cgi?id=541653
2. Click inside the browser window at the background.
3. Click again.
4. Click at "Edit" in Menu.


Actual results:

Browser does not open the menu.


Expected results:

Open the menu.

Comment 1

[Thomas Ahlblom]

WFM:
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110731 Firefox/7.0a2

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110801 Firefox/8.0a1


Last good nightly: 2011-07-24
First bad nightly: 2011-07-25

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad1655c2e5b1&tochange=4f38df646524

Comment 2

[Thomas Ahlblom]

Refined regression range using hourly builds:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=99644756e69e&tochange=de43fbe04808

Comment 3

[Thomas Ahlblom]

Local track down:

The first bad revision is:
changeset:   73238:de43fbe04808
user:        Masayuki Nakano <[e-mail]>
date:        Sun Jul 24 23:26:40 2011 +0900
summary:     Bug 673315 part.1 selection change reason must be MOUSEUP_REASON when dragging for selection finishes normally r=smaug

Comment 4

[Thomas Ahlblom]

It is possible to unlock the hang by middle-clicking in Linux, and that's sounds related to the clipboard problems in bug 673315.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is very interesting bug for me.

By the landed patch, PresShell fails to release mouse capture at mouseup event because nsEventStateManager::PostHandleEvent() makes a modal dialog via click event before releasing mouse capture.

I'll fix this bug by a simple patch for risk management. However, I guess that there is a hidden critical bug in focus management.

Comment 6

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

Since this is pretty serious bug, perhaps back out  Bug 673315 ?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm now testing a patch for this in tryserver...
http://hg.mozilla.org/try/rev/7e9ca31a0db3

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think this should prevent this bug, however, it doesn't work:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6445

because there is no active ESM at that time. The sActiveESM is cleared by ESM itself at PostHandleEvent() before it calls CheckForAndDispatchClick(). So, seems it doesn't assume that mouse capture isn't released before it's called.

This was hidden even before bug 552707. There were some frames which overrode HandleRelease() but didn't release mouse capture.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This patch calls SetCapturingContent(nsnull, 0) from nsFrameSelection::SetMouseDownState(PR_FALSE) if the instance is handling drag.

And also this patch gets back SetCapturingContent(nsnull, 0) to nsFrame for decreasing risk of other regressions which hasn't been found yet. This guarantees same behavior before bug 673315 to the methods which will be called after nsFrame::HandleEvent().

Now, all tests passed on Linux x86-64. Let's wait the other results on platforms.

Comment 10

[Stefan]

A build of curren rev. 3735fb1cd5ef does not reproduce the issue. Is this a consequence of this backout?:

http://hg.mozilla.org/mozilla-central/rev/d32bc792fb5e

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, I forgot to change this bug, so, yes, it is. But don't close this bug, the patches will be back. I need to fix this bug.

Comment 12

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

Comment on attachment 549872 [details] [diff] [review]
Patch

I guess we should restrict capturing content to the focused window,
and when it get blur'ed,  capturing content should be set null.

Enn might have other ideas.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Sure. I think that this is the strict fix.

Comment 14

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

I was thinking to add something to focusmanager.

Btw, why does the patch fix the problem, why activeShell->SetCapturingContent(nsnull, 0); isn't enough?

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Since activeESM is null at that time. nsEventStateManager::PostHandleEvent() calls ClearGlobalActiveContent() at mouseup event.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3164

After this, ESM dispatches click event. Therefore, there is no active ESM when click event is handled.

Comment 16

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

Comment on attachment 551431 [details] [diff] [review]
Patch

Though, I believe focusmanager should also release capturing content in certain
cases. But that is a different bug.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Resetting assignee which I don't work on in this several months.

Comment 18

[Andrei Purice]

Hey Stefan,
Can you still reproduce this issue or should we close it?

Comment 19

[Andrei Purice]

Marking this as Resolved > Incomplete since there are no new updates from the reporter.
Feel free to reopen it if the issue still occurs.