1439246 - Navigation does not clear browser nor page actions in inactive tabs

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: test.xpi

Bug 1395074 enabled automatic clearance of browser actions on navigation.

However, this only happens if the navigation takes place in the active tab.

1. Install attached webextension in about:debugging
2. Load http://example.com
3. Click the browser action. This shows a badge only for that tab.
4. Open web console and run
   setTimeout(() => { location.href = '/foo' }, 3e3);
5. Create a new tab before 3 seconds
6. Wait until the old tab will navigates
7. Switch back to the old tab, it still has the badge

Result: the badge should have been cleared on navigation, just like Chrome, or just like if the tab were active.

Page actions should have the same problem since their implementation in bug 1197422.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Review commit: https://reviewboard.mozilla.org/r/221238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221238/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

https://reviewboard.mozilla.org/r/221238/#review227064


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/ext-pageAction.js:297
(Diff revision 1)
> +      // Clear tab data on navigation.
>        this.tabContext.clear(tab);
> +    } else if (fromBrowse === false) {
> +      // Clear pattern matching cache when URL changes.
> +      let tabData = this.tabContext.get(tab);
> +      if ('patternMatching' in tabData) {

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_pageAction_show_matches.js:173
(Diff revision 1)
> +  let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, url1, true, true);
> +
> +  info("Perform navigation in the old tab");
> +  await setUrl(tab, url1);
> +  await sendMessage(extension, "isShown", {tabId: getId(tab)}, true,
> +    "Navigating undoes hide(), even when the tab is not selected.");

Error: Expected indentation of 20 spaces but found 4. [eslint: indent]

Comment 3

[Oriol Brufau [:Oriol]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221238/diff/1-2/

Comment 4

[Shane Caraveo (:mixedpuppy)]

I haven't fully looked at the patch, but my initial reaction is that it would be better to update on TabSwitch rather than every change.

Comment 5

[Oriol Brufau [:Oriol]]

Note that browserAction already only updates if the tab is selected (https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/browser/components/extensions/ext-browserAction.js#541), and I added the same check to pageAction. So the patch will clear the tab data on every change, but only update the button if the tab is selected.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

https://reviewboard.mozilla.org/r/221238/#review229424

Adding Kris to see if he has any thought on how to avoid the extra location-change events for all tabs.  I'm pretty ok with this given my comment, but I'm not real happy with the extra events.

::: browser/components/extensions/ext-browser.js
(Diff revision 2)
>      }
>    }
>  
>    onLocationChange(browser, webProgress, request, locationURI, flags) {
>      let gBrowser = browser.ownerGlobal.gBrowser;
> -    if (browser === gBrowser.selectedBrowser) {

I am bothered by having the extra events, however, what is not said in the bug (that I realized after trying something slightly different) is that and get api calls made on a non-active tab could have the wrong data returned.  Reseting the tabcontext for that is necessary.

::: browser/components/extensions/ext-pageAction.js:286
(Diff revision 2)
>        TelemetryStopwatch.cancel(popupOpenTimingHistogram, this);
>        this.emit("click", tab);
>      }
>    }
>  
>    handleLocationChange(eventType, tab, fromBrowse) {

This could probably use a high level comment.  Something along the lines of: We need to update the tabData for any location change, however we only update the button when the selected tab has a location change, or the selected tab has changed.

::: browser/components/extensions/ext-pageAction.js:289
(Diff revision 2)
>    }
>  
>    handleLocationChange(eventType, tab, fromBrowse) {
>      // fromBrowse can have three values:
> -    // - true: navigation occurred in the active tab
> -    // - false: the location changed in the active tab but no navigation occurred
> +    // - true: navigation occurred
> +    // - false: the location changed but no navigation occurred

comment on why.  e.g. history pushstate.

::: browser/components/extensions/ext-pageAction.js:290
(Diff revision 2)
>  
>    handleLocationChange(eventType, tab, fromBrowse) {
>      // fromBrowse can have three values:
> -    // - true: navigation occurred in the active tab
> -    // - false: the location changed in the active tab but no navigation occurred
> +    // - true: navigation occurred
> +    // - false: the location changed but no navigation occurred
>      // - undefined: an inactive tab has become active

"undefined: TabSelect has occurred, tabData does not need to be updated."

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:613
(Diff revision 2)
> +  async function locationChange(tab, url, task) {
> +    let locationChanged = BrowserTestUtils.waitForLocationChange(gBrowser, url);
> +    await ContentTask.spawn(tab.linkedBrowser, url, task);
> +    await locationChanged;
> +  }
> +  function setUrl(tab, url) {
> +    return locationChange(tab, url, (url) => { content.location.href = url; });
> +  }
> +  function historyPushState(tab, url) {
> +    return locationChange(tab, url, (url) => { content.history.pushState(null, null, url); });
> +  }

These are repeated in more than one test file, move them to head.js

Comment 7

[Oriol Brufau [:Oriol]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221238/diff/2-3/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

or r? zombie

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Comment on attachment 8952019 [details]
> Bug 1439246 - Clear browser and page actions when inactive tabs navigate
> 
> https://reviewboard.mozilla.org/r/221238/#review229424

> I am bothered by having the extra events, however, what is not said in the
> bug (that I realized after trying something slightly different) is that and
> get api calls made on a non-active tab could have the wrong data returned. 
> Reseting the tabcontext for that is necessary.

"is that any get api calls made"

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> I am bothered by having the extra events, however, what is not said in the
> bug (that I realized after trying something slightly different) is that [any]
> get api calls made on a non-active tab could have the wrong data returned. 
> Reseting the tabcontext for that is necessary.

I'm not sure this is a big deal, I can't see a use case when an extension would need to know up-to-date browserAction badge test for a non-selected tab.

Comment 11

[Tomislav Jovanovic :zombie]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

https://reviewboard.mozilla.org/r/221238/#review234442

If we want to guarantee the `getBadgeText()` behavior above (which I'm not convinced, but meh), then I believe handling all navigation events from all tabs is necesery (which is a shame).

Otherwise looks good, with a few nits.

::: browser/components/extensions/ext-pageAction.js:289
(Diff revision 3)
>    }
>  
> +  // We need to update the tabData for any location change, however we only update the
> +  // button when the selected tab has a location change, or the selected tab has changed.
>    handleLocationChange(eventType, tab, fromBrowse) {
>      // fromBrowse can have three values:

nit: this would be better as jsdoc.

::: browser/components/extensions/ext-pageAction.js:300
(Diff revision 3)
>        this.tabContext.clear(tab);
> +    } else if (fromBrowse === false) {
> +      // Clear pattern matching cache when URL changes.
> +      let tabData = this.tabContext.get(tab);
> +      if ("patternMatching" in tabData) {
> +        tabData.patternMatching = undefined;

`"patternMatching" in tabData` will still be true after this.  Didn't look further into details, but it looks wrong at least.

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:678
(Diff revision 3)
> +  info("Set tab-specific data.");
> +  await setTabSpecificData(tab2);
> +
> +  info("Push history state. Does not cause navigation.");
> +  await historyPushState(tab2, url + "/path");
> +  await expectTabSpecificData(tab2, "history.pushState() does not clear tab-specific data");

What's the reason for this?  Is it documented somewhere as expected behavior, or is it something that we should perhaps change?

::: browser/components/extensions/test/browser/head.js:510
(Diff revision 3)
>  }
> +
> +async function locationChange(tab, url, task) {
> +  let locationChanged = BrowserTestUtils.waitForLocationChange(gBrowser, url);
> +  await ContentTask.spawn(tab.linkedBrowser, url, task);
> +  await locationChanged;

nit: you can just `return locationChanged` here.

::: browser/components/extensions/test/browser/head.js:512
(Diff revision 3)
> +async function locationChange(tab, url, task) {
> +  let locationChanged = BrowserTestUtils.waitForLocationChange(gBrowser, url);
> +  await ContentTask.spawn(tab.linkedBrowser, url, task);
> +  await locationChanged;
> +}
> +function setUrl(tab, url) {

Now that this exported, a bit more descriptive name like "navigateTab" would be better.  Also, empty lines between functions

Comment 12

[Oriol Brufau [:Oriol]]

(In reply to Tomislav Jovanovic :zombie from comment #11)
> ::: browser/components/extensions/ext-pageAction.js:300
> (Diff revision 3)
> >        this.tabContext.clear(tab);
> > +    } else if (fromBrowse === false) {
> > +      // Clear pattern matching cache when URL changes.
> > +      let tabData = this.tabContext.get(tab);
> > +      if ("patternMatching" in tabData) {
> > +        tabData.patternMatching = undefined;
> 
> `"patternMatching" in tabData` will still be true after this.  Didn't look
> further into details, but it looks wrong at least.

My reasoning was that, in most cases, if we had a patternMatching cache, then we will still need it. So I thought that using the `delete` operator was not worth, because I read that it may prevent some JS optimizations, and most probably patternMatching will be recreated immediately after in the isShown function. So I just set it to `undefined`. However, in case patternMatching did not exist (e.g. show_matches is empty), I didn't want to add unnecessary properties, and that's why the condition uses the `in` operator.

Should I add a comment explaining this? Do you prefer `if (tabData.patternMatching !== undefined)`, which will also imply the property exists? Or just `delete tabData.patternMatching` if you think the above is an unnecessary micro-optimization?

> :::
> browser/components/extensions/test/browser/browser_ext_browserAction_context.
> js:678
> (Diff revision 3)
> > +  info("Set tab-specific data.");
> > +  await setTabSpecificData(tab2);
> > +
> > +  info("Push history state. Does not cause navigation.");
> > +  await historyPushState(tab2, url + "/path");
> > +  await expectTabSpecificData(tab2, "history.pushState() does not clear tab-specific data");
> 
> What's the reason for this?  Is it documented somewhere as expected
> behavior, or is it something that we should perhaps change?

It's what Chrome does, it's easier to implement, and it's what makes sense IMO. See bug 1438274.

Comment 13

[Oriol Brufau [:Oriol]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221238/diff/3-4/

Comment 14

[Oriol Brufau [:Oriol]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Reviewboard always messes up the review flags...

Comment 15

[Tomislav Jovanovic :zombie]

(In reply to Oriol Brufau [:Oriol] from comment #12)
> Do you prefer `if (tabData.patternMatching !== undefined)`, 

We are already comparing it to undefined in other places, so let's stay consistent with that, it's fairly obvious what it means.

Comment 16

[Tomislav Jovanovic :zombie]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

https://reviewboard.mozilla.org/r/221238/#review234450

Comment 17

[Oriol Brufau [:Oriol]]

Comment on attachment 8952019 [details]
Bug 1439246 - Clear browser and page actions when inactive tabs navigate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221238/diff/4-5/

Comment 18

[Oriol Brufau [:Oriol]]

Try seems green enough.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f1853f58896
Clear browser and page actions when inactive tabs navigate r=mixedpuppy,zombie

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8f1853f58896

Comment 21

[CosminB]

Attachment: Bug1439246.gif

I can reproduce the issue on Firefox 59.0.2 (20180323154952) under Win 7 64-bit and Mac OS X 10.13.2 with the extension from the Description.

The badge is not cleared on navigation.

This issue is verified as fixed on Firefox 60.0a1 (20180404100127) under Win 7 64-bit and Mac OS X 10.13.2.

Please see the attached video.