Pres Shell mouse capture leaks when no mouse up received (split mochitests 2/5 leaks on Windows)

RESOLVED FIXED

Status

()

Core
Event Handling
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dbaron, Assigned: smaug)

Tracking

({mlk})

Trunk
x86
Windows Server 2003
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
This morning, releng landed split mochitests, so that mochitests are separated into 5 parts.

The remaining regression from this is that there's a substantial leak on Windows only, on part 2/5 of the mochitests.  It shows up on the old-style mochitests and the debug mochitests.

I can reproduce the leak locally by running all of part 2/5 (--total-chunks=5 --this-chunk=2 --chunk-by-dir=4) or just by running with --test-path=dom/tests/mochitest/bugs/
(Reporter)

Comment 1

8 years ago
Note that we tried increasing the cycle collector shutdown collections limit from 5 to 10, and it didn't help:
http://hg.mozilla.org/mozilla-central/rev/d6add78eff55
http://hg.mozilla.org/mozilla-central/rev/11a42c6d5aa8
http://hg.mozilla.org/mozilla-central/rev/69a4a21d9b84
(Reporter)

Comment 2

8 years ago
It would not surprise me if this is related to bug 523885.
(Reporter)

Comment 3

8 years ago
I think my first guess at what to take a refcnt log of might have been right:

I logged reference counts on the one nsHTMLHtmlElement leaked.  There was one outstanding reference, and it looks like it was the one taken in nsIPresShell::SetCapturingContent (code added last month in bug 503943, http://hg.mozilla.org/mozilla-central/rev/eda2433181c9 ).

But I'm going to sleep now.
(Reporter)

Comment 4

8 years ago
(And the leak as a result of splitting sort of makes sense for this leak, since this would leak until somebody called SetCapturingContent on some other piece of content; in parts 3, 4, or 5, there's probably a test that also does capturing and therefore lets the memory for this test get freed.)

Comment 5

8 years ago
The capturing content is a global and should be cleared when either the mouse is released, the view containing it is destroyed or hidden, or when any window is deactivated. Is nsIPresShell::gCaptureInfo::mContent still set at the end of the test run?

There's only one test that calls setCapture directly: toolkit/content/tests/widgets/test_mousecapture.xul

But setCapture can be called directly by other elements though, for example: <select>, <splitter>, <frameset>. If it's an nsHTMLHtmlElement element that's leaking though it seems like it might be a selection drag capture.
(Reporter)

Comment 6

8 years ago
The capturing content is set to the root element of the *harness* page while running dom/tests/mochitest/bugs/test_bug397571.html and stays that way until the end of the mochitest run (the point of the last call to ~PresShell).
(Reporter)

Comment 7

8 years ago
I just landed a *workaround* for this leak:
http://hg.mozilla.org/mozilla-central/rev/3e855f1a3f75
though we should still find a real fix for it.
(Reporter)

Comment 8

8 years ago
(I don't know whether it's possible to trigger it without nsIDOMWindowUtils.sendMouseEvent, though.)
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
(Reporter)

Updated

8 years ago
Component: General → Event Handling
QA Contact: general → events
Summary: split mochitests 2/5 leaks on Windows → Pres Shell mouse capture leaks when no mouse up received (split mochitests 2/5 leaks on Windows)

Comment 9

8 years ago
OK, test_bug397671 runs near the end and no mouse related tests run after it, which is why the capture is still active, as no mouseup ever occurred. The patch here is correct as the test should be firing a mouseup as well.

The test_bug397671 runs during inline script which may be occurring before the widget hierarchy is set properly perhaps, thus attempting to get the widget to fire the mousedown event at during nsIDOMWindowUtils.sendMouseEvent retrieves the parent instead.
(Reporter)

Comment 10

8 years ago
Is it possible to hit this leak as a result of user action, or can it only result from nsIDOMWindowUtils event synthesis?  (What about standard DOM event synthesis?  Hopefully we're checking that the events are trusted...)

Comment 11

8 years ago
(In reply to comment #10)
> Is it possible to hit this leak as a result of user action, or can it only
> result from nsIDOMWindowUtils event synthesis?  (What about standard DOM event
> synthesis?  Hopefully we're checking that the events are trusted...)

Would be worth testing but I don't think there's a problem.

Real user events should always have the right widget set from the native widget code. Also, the user can release the capture by just letting go of the mouse button ;)

Synthetic DOM events shouldn't be a problem either. Mouse capture is only allowed to be enabled during a real mousedown event (enabled in PresShell::HandleEventInternal)
(Reporter)

Comment 12

8 years ago
Then I think we should at least add assertions to DOMWindowUtils to make sure that mousedowns are paired with mouseups.
Created attachment 408008 [details]
testcase?

This shows a problem with our mousedown/up handling.
And it also shows that mousedown without mouseup is possible, and that is why
nsIDOMWindowUtils shouldn't assert about it.
Note, I'm using linux with the testcase. Not sure what other OSes do.

Comment 15

8 years ago
The capture is also released when a window is deactivated so the testcase shouldn't cause a problem there. It may be that a mouseup should be fired at the document anyway though.
I don't know that I can say that this is really a release blocker, but fixing the leaks in our tests would be a really good thing to do. Marking a blocker for now, to make sure this doesn't get lost...

Olli, can you investigate a bit more here?
Assignee: nobody → Olli.Pettay
blocking2.0: ? → beta1

Updated

8 years ago
blocking2.0: beta1+ → beta2+

Updated

8 years ago
blocking2.0: beta2+ → betaN+
(In reply to comment #15)
> The capture is also released when a window is deactivated
Actually this doesn't seem to work. Investigating.
Ok, so seems like the problem with attachment 408008 [details] is that in that case
we set the capturing content right *after* dismissing the alert. So 
deactivating the window and activating alert happens before setting
capturing content. And the mouseup is processed before we set the capturing content.
A bit tricky case. Need to play a bit with nsEventStateManager::mNormalLMouseEventInProcess.
Created attachment 472471 [details] [diff] [review]
patch

So before setting capturing content, nsEventStateManager::EventStatusOK is called.
If we make sNormalLMouseEventInProcess global, this should fix the problem, 
at least for this case.

I posted the patch to tryserver.

And note, if a testcase sends a mousedown, without mouseup, it is "ok" to
keep the capturing content, IMHO.
Attachment #472471 - Flags: review?(enndeakin)
The mochitest leak part of this patch should have been fixed in Bug 528829.
(In reply to comment #20)
> The mochitest leak part of this patch should have been fixed in Bug 528829.
...if this bug ...
And still wrong.
...of this bug ...

Updated

7 years ago
Attachment #472471 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/a3f6cd765777
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.