Properly remove mouseover listeners from tabs that don't need them anymore

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: dao, Assigned: beekill)

Tracking

(Depends on: 1 bug, {regression})

Trunk
Firefox 57
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
(In reply to Mike de Boer [:mikedeboer] from bug 874533 comment 7)
> > +              let url = tabData.entries[activeIndex].url;
> > +              this.prepareConnectionToHost(url);
> > +            }
> > +          };
> > +          tab.addEventListener("mouseover", tab.__onMouseOver);
> 
> In that function, please add `, {once: true}` to cleanup the listener
> properly.

That's not enough. Tabs may be selected with the keyboard, they may not even see a mouseover event throughout their whole lifetime.
Flags: needinfo?(nnn_bikiu0707)
Comment hidden (mozreview-request)
(Reporter)

Comment 2

6 months ago
mozreview-review
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

https://reviewboard.mozilla.org/r/160878/#review166146

The tab may also be closed without the mouse before it gets selected. How about we call speculativeConnectOnTabHover from tabbrowser.xml (_mouseenter)?
Attachment #8889786 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 3

6 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> Comment on attachment 8889786 [details]
> Bug 1383073 - Remove mouseover listeners from tabs that are selected.
> 
> https://reviewboard.mozilla.org/r/160878/#review166146
> 
> The tab may also be closed without the mouse before it gets selected. 

I don't know what is wrong with tabs are closed without the mouse. I mean, aren't all event handlers are destroyed with the tab and we don't have to worry about them?

> How about we call speculativeConnectOnTabHover from tabbrowser.xml (_mouseenter)?

We only register the mouseenter event when we're restoring on demand. I think from tabbrowser.xml, there's no way we can tell whether we are restoring on demand or not. Besides, we have to find out a way to tell whether a connection is prepared and we don't have to call the function again, which can complicate thing. Maybe a flag can solve it, however, I think it's the best to keep the speculativeConnect in SessionStore.jsm.
Flags: needinfo?(nnn_bikiu0707) → needinfo?(dao+bmo)
(Reporter)

Comment 4

6 months ago
(In reply to Bao Quan [:beekill] from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > Comment on attachment 8889786 [details]
> > Bug 1383073 - Remove mouseover listeners from tabs that are selected.
> > 
> > https://reviewboard.mozilla.org/r/160878/#review166146
> > 
> > The tab may also be closed without the mouse before it gets selected. 
> 
> I don't know what is wrong with tabs are closed without the mouse. I mean,
> aren't all event handlers are destroyed with the tab and we don't have to
> worry about them?

No, the event handlers will stay around and keep the tab node alive. :/

> > How about we call speculativeConnectOnTabHover from tabbrowser.xml (_mouseenter)?
> 
> We only register the mouseenter event when we're restoring on demand. I
> think from tabbrowser.xml, there's no way we can tell whether we are
> restoring on demand or not.

Yes, there is: the "pending" attribute on the tab. But tabbrowser.xml doesn't even have to worry about that, it could just call SessionStore and SessionStore could decide itself whether to call speculativeConnect.
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

6 months ago
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

https://reviewboard.mozilla.org/r/160878/#review166686

::: browser/components/sessionstore/SessionStore.jsm:3456
(Diff revision 2)
>     * @param tab
>     *        A tab to set up a hover listener.
>     * @param url
>     *        URL of a host.
>     */
>    speculativeConnectOnTabHover(tab, url) {

You can just call getLazyTabValue(tab, "url") here so tabbrowser doesn't have to create an nsIURI for browser.currentURI.spec.

::: browser/components/sessionstore/SessionStore.jsm:3457
(Diff revision 2)
>     *        A tab to set up a hover listener.
>     * @param url
>     *        URL of a host.
>     */
>    speculativeConnectOnTabHover(tab, url) {
> -    tab.addEventListener("mouseover", () => {
> +    if (tab.__SS_shouldPrepareConnection) {

Can you check the pending attribute instead? This way we don't need __SS_shouldPrepareConnection at all.

::: browser/components/sessionstore/SessionStore.jsm:3463
(Diff revision 2)
> +      delete tab.__SS_shouldPrepareConnection;
>        let prepared = this.prepareConnectionToHost(url);
>        // This is used to test if a connection has been made beforehand.
>        if (gDebuggingEnabled) {
> +        // This is used to test if a connection has been made beforehand.
> +        if (gDebuggingEnabled) {

You already checked gDebuggingEnabled above.
Attachment #8889786 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 7

6 months ago
(In reply to Dão Gottwald [::dao] from comment #6)
> ::: browser/components/sessionstore/SessionStore.jsm:3457
> (Diff revision 2)
> >     *        A tab to set up a hover listener.
> >     * @param url
> >     *        URL of a host.
> >     */
> >    speculativeConnectOnTabHover(tab, url) {
> > -    tab.addEventListener("mouseover", () => {
> > +    if (tab.__SS_shouldPrepareConnection) {
> 
> Can you check the pending attribute instead? This way we don't need
> __SS_shouldPrepareConnection at all.

You mean this [1]. If I understand the attribute correctly, the attribute is not removed until the tab is restored. During that period, a tab might be hovered many times and therefore, we'll call the function many times unnecessarily.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/pending
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 8

6 months ago
(In reply to Bao Quan [:beekill] from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > ::: browser/components/sessionstore/SessionStore.jsm:3457
> > (Diff revision 2)
> > >     *        A tab to set up a hover listener.
> > >     * @param url
> > >     *        URL of a host.
> > >     */
> > >    speculativeConnectOnTabHover(tab, url) {
> > > -    tab.addEventListener("mouseover", () => {
> > > +    if (tab.__SS_shouldPrepareConnection) {
> > 
> > Can you check the pending attribute instead? This way we don't need
> > __SS_shouldPrepareConnection at all.
> 
> You mean this [1]. If I understand the attribute correctly, the attribute is
> not removed until the tab is restored. During that period, a tab might be
> hovered many times and therefore, we'll call the function many times
> unnecessarily.

Okay, so let's make speculativeConnectOnTabHover set a flag when we connect speculatively, and check that flag in addition to the pending attribute.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 9

6 months ago
(In reply to Dão Gottwald [::dao] from comment #8)
> Okay, so let's make speculativeConnectOnTabHover set a flag when we connect
> speculatively, and check that flag in addition to the pending attribute.

I'm sorry to bother you, Dão. But I don't understand why we need to check the pending attribute? Wouldn't check the flag is enough?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 10

6 months ago
(In reply to Bao Quan [:beekill] from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > Okay, so let's make speculativeConnectOnTabHover set a flag when we connect
> > speculatively, and check that flag in addition to the pending attribute.
> 
> I'm sorry to bother you, Dão. But I don't understand why we need to check
> the pending attribute? Wouldn't check the flag is enough?

No, what I'm proposing is different from your current patch. restoreWindow should not set a flag. Instead, speculativeConnectOnTabHover should set a flag to prevent speculatively connecting a second time.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

6 months ago
(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to Bao Quan [:beekill] from comment #9)
> > (In reply to Dão Gottwald [::dao] from comment #8)
> > > Okay, so let's make speculativeConnectOnTabHover set a flag when we connect
> > > speculatively, and check that flag in addition to the pending attribute.
> > 
> > I'm sorry to bother you, Dão. But I don't understand why we need to check
> > the pending attribute? Wouldn't check the flag is enough?
> 
> No, what I'm proposing is different from your current patch. restoreWindow
> should not set a flag. Instead, speculativeConnectOnTabHover should set a
> flag to prevent speculatively connecting a second time.

Got it! :)
(Reporter)

Comment 13

6 months ago
mozreview-review
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

https://reviewboard.mozilla.org/r/160878/#review167738

::: browser/components/sessionstore/SessionStore.jsm:3455
(Diff revision 3)
> -   * @param url
> -   *        URL of a host.
>     */
> -  speculativeConnectOnTabHover(tab, url) {
> -    tab.addEventListener("mouseover", () => {
> +  speculativeConnectOnTabHover(tab) {
> +    if (this._restore_on_demand && !tab.__SS_connectionPrepared &&
> +        tab.hasAttribute("pending") && !tab.hasAttribute("pinned")) {

You can remove the this._restore_on_demand and !tab.hasAttribute("pinned") checks. tab.hasAttribute("pending") makes them redundant.

::: browser/components/sessionstore/test/browser_speculative_connect.js:46
(Diff revision 3)
>  
>    // Trigger a mouse enter on second tab.
>    tabs[1].dispatchEvent(e);
>    is(tabs[1].__test_connection_prepared, false, "Second tab doesn't have a connection prepared");
>    is(tabs[1].__test_connection_url, TEST_URLS[0], "Second tab has correct url");
> +  is(tabs[1].__SS_connectionPrepared, true, "Second tab should have __SS_connectionPrepared flag after hovered");

Can you make the test use the speculative-connect-request notification instead of these flags?
Attachment #8889786 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 14

6 months ago
(In reply to Dão Gottwald [::dao] from comment #13)
> ::: browser/components/sessionstore/test/browser_speculative_connect.js:46
> (Diff revision 3)
> >  
> >    // Trigger a mouse enter on second tab.
> >    tabs[1].dispatchEvent(e);
> >    is(tabs[1].__test_connection_prepared, false, "Second tab doesn't have a connection prepared");
> >    is(tabs[1].__test_connection_url, TEST_URLS[0], "Second tab has correct url");
> > +  is(tabs[1].__SS_connectionPrepared, true, "Second tab should have __SS_connectionPrepared flag after hovered");
> 
> Can you make the test use the speculative-connect-request notification
> instead of these flags?

Dão, what is a speculative-connect-request notification? Is it some kind of custom event like this [1]?

[1]: https://developer.mozilla.org/en/docs/Web/API/CustomEvent
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 15

6 months ago
(In reply to Bao Quan [:beekill] from comment #14)
> Dão, what is a speculative-connect-request notification? Is it some kind of
> custom event like this [1]?
> 
> [1]: https://developer.mozilla.org/en/docs/Web/API/CustomEvent

It's an nsIObserver notification. Here's an example use:

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/netwerk/test/mochitests/test_rel_preconnect.html#20-31
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 16

6 months ago
Responding to your question from IRC.

<beekill> Hi Dão, I want to ask you about "speculative-connect-request". It's look like we don't know about the subject of the notification: http://searchfox.org/mozilla-central/search?q=speculative-connect-request&case=false&regexp=false&path=
which means we can only count the number of speculative connections have been made
I wonder if that is enough for the test?
<dao> I think you can do better than just counting the number of requests. after dispatchEvent(e) you should use await TestUtils.topicObserved before you make the next dispatchEvent call
(Assignee)

Comment 17

6 months ago
mozreview-review-reply
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

https://reviewboard.mozilla.org/r/160878/#review167738

> You can remove the this._restore_on_demand and !tab.hasAttribute("pinned") checks. tab.hasAttribute("pending") makes them redundant.

I did some testing about this. When we're not restoring on demand, the pending attribute is in the tabs that are not restored. I think we should check `this._restore_on_demand` in addition to pending attribute.

> Can you make the test use the speculative-connect-request notification instead of these flags?

I put some logging in nsHttpHandler [1] and nsIOService [2]. The speculative connect in nsIOService doesn't call nsHttpHandler or vice versa. I guess the only way to test this is to check those flags.

[1]: http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2404
[2]: http://searchfox.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1825
Comment hidden (mozreview-request)
(Reporter)

Comment 19

6 months ago
(In reply to Bao Quan [:beekill] from comment #17)
> Comment on attachment 8889786 [details]
> Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we
> don't have to worry about clearing mouseover listeners from tabs. .
> 
> https://reviewboard.mozilla.org/r/160878/#review167738
> 
> > You can remove the this._restore_on_demand and !tab.hasAttribute("pinned") checks. tab.hasAttribute("pending") makes them redundant.
> 
> I did some testing about this. When we're not restoring on demand, the
> pending attribute is in the tabs that are not restored.

Why would we not want to connect speculatively in that case?
(Assignee)

Comment 20

6 months ago
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Bao Quan [:beekill] from comment #17)
> > Comment on attachment 8889786 [details]
> > Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we
> > don't have to worry about clearing mouseover listeners from tabs. .
> > 
> > https://reviewboard.mozilla.org/r/160878/#review167738
> > 
> > > You can remove the this._restore_on_demand and !tab.hasAttribute("pinned") checks. tab.hasAttribute("pending") makes them redundant.
> > 
> > I did some testing about this. When we're not restoring on demand, the
> > pending attribute is in the tabs that are not restored.
> 
> Why would we not want to connect speculatively in that case?

We did connect speculatively in that case. We don't wait until the users hover their mouse on tabs, instead, we speculatively connect when we're restoring: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3728-3739
(Reporter)

Comment 21

6 months ago
Ah, okay.
(Reporter)

Comment 22

6 months ago
mozreview-review
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

https://reviewboard.mozilla.org/r/160878/#review170320
Attachment #8889786 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 23

5 months ago
The try server result looks good. Can you push this, Dão? Thanks!
Flags: needinfo?(dao+bmo)

Comment 24

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc352ddd6868
Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. r=dao.
(Reporter)

Updated

5 months ago
Flags: needinfo?(dao+bmo)

Comment 25

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc352ddd6868
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox57: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Comment 26

5 months ago
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

Approval Request Comment
[Feature/Bug causing the regression]: bug 874533
[User impact if declined]: the event listener could keep the tab node alive after it got removed which is basically a small memory leak
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: little risk
[Why is the change risky/not risky?]: the fix is fairly straightforward and there resulting code should overall be more robust
[String changes made/needed]: /
Attachment #8889786 - Flags: approval-mozilla-beta?
status-firefox56: --- → affected
Keywords: regression
status-firefox55: --- → unaffected
Comment on attachment 8889786 [details]
Bug 1383073 - Move speculativeConnectOnTabHover to tabbrowser.xml so we don't have to worry about clearing mouseover listeners from tabs. .

Fix for memory leak, let's uplift this for beta 3.
Attachment #8889786 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 28

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/edbb6431090c
status-firefox56: affected → fixed

Updated

a month ago
Depends on: 1418009
You need to log in before you can comment on or make changes to this bug.