Closed Bug 1383073 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: dao, Assigned: beekill)

References

Details

(Keywords: regression)

Attachments

(1 file)

(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 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-
(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)
(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)
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
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-
(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)
(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)
(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)
(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)
(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! :)
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-
(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)
(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)
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
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
(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?
(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
Ah, okay.
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+
The try server result looks good. Can you push this, Dão? Thanks!
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
https://hg.mozilla.org/mozilla-central/rev/cc352ddd6868
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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?
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+
Depends on: 1418009
You need to log in before you can comment on or make changes to this bug.