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

Status: RESOLVED FIXED

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

Description

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

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/

Comment 1

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

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

Comment 2

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

It would not surprise me if this is related to bug 523885.

Comment 3

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

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.

Comment 4

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

(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

[Neil Deakin]

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.

Comment 6

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

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

Comment 7

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

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.

Comment 8

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

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

Comment 9

[Neil Deakin]

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.

Comment 10

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

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

[Neil Deakin]

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

Comment 12

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

Then I think we should at least add assertions to DOMWindowUtils to make sure that mousedowns are paired with mouseups.

Comment 13

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

Attachment: 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.

Comment 14

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

Note, I'm using linux with the testcase. Not sure what other OSes do.

Comment 15

[Neil Deakin]

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.

Comment 16

[Johnny Stenback (:jst)]

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?

Comment 17

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

(In reply to comment #15)
> The capture is also released when a window is deactivated
Actually this doesn't seem to work. Investigating.

Comment 18

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

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.

Comment 19

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

Attachment: 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.

Comment 20

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

The mochitest leak part of this patch should have been fixed in Bug 528829.

Comment 21

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

(In reply to comment #20)
> The mochitest leak part of this patch should have been fixed in Bug 528829.
...if this bug ...

Comment 22

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

And still wrong.
...of this bug ...

Comment 23

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

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