If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

WebExtension tests should not rely on tabs.onUpdated inside a callback

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

a year ago
There are multiple unit tests* that use the following pattern:

let url = ...
let resolve = ...
browser.test.create({url}, tab => {
  browser.tabs.onUpdated.addListener(function listener(tabId_, changeInfo) {
    if (changeInfo.status == "complete" && tabId == tabId_) {
      browser.tabs.onUpdated.removeListener(listener);
      resolve();
    }
  });
});

This should NOT be used because there is no guarantee that the asynchronous callback is triggered before the onUpdated event is fired.

In fact, when we move the tabs API to ChildAPIManager/ParentAPIManager (bug 1287007), the likelihood of intermittents increases because there is a longer delay between the callback firing and the listener getting added.

Many extension developers actually use the above pattern, but it cannot be relied upon, *even in Chrome* (see https://crbug.com/411225). A work-around (for Chrome) is given in http://stackoverflow.com/questions/27405698/chrome-development-chrome-tabs-sendmessage-not-notifying-runtime/27408542#27408542.

Another alternative is to first register the onUpdated listener and then calling tabs.create or tabs.update.


* As of writing there are 17 results for tabs.onUpdated.addListener [1], the following tests are affected (linking to first occurrence of onUpdated):
- http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js#55
- http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/browser/components/extensions/test/browser/browser_ext_tabs_events.js#246
- http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js#113
- http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/browser/components/extensions/test/browser/browser_ext_tabs_reload_bypass_cache.js#17
- http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/browser/components/extensions/test/browser/browser_ext_tabs_zoom.js#18 (this is not inside a callback, but onUpdated is registered right after calling tabs.update)
- http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js#18

[1] http://searchfox.org/mozilla-central/search?q=tabs.onUpdated.addListener&path=components%2Fextensions%2Ftest
(Assignee)

Comment 1

a year ago
More alternatives:
- Do not start loading the tab until the callback has fired (bad idea)
- Add a new attribute to tabs.create / tabs.update to not invoke the callback until changeInfo.status == "complete", since this is a very common use case.

I like the second one.
Comment hidden (mozreview-request)

Updated

a year ago
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Updated

a year ago
Blocks: 1287007
See Also: bug 1287007
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

https://reviewboard.mozilla.org/r/78038/#review78094

::: browser/components/extensions/ext-tabs.js:775
(Diff revision 3)
>        },
>  
>        detectLanguage: function(tabId) {
>          let tab = tabId !== null ? TabManager.getTab(tabId, context) : TabManager.activeTab;
>  
> +        return tabListener.awaitTabReady(self.tabs._detectLanguage, tab);

This is way more complicated than it needs to be. `return tabListener.awaitTabReady(tab).then(() => { ... });`

Same for the other calls.
Attachment #8790061 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

https://reviewboard.mozilla.org/r/78038/#review80820
Attachment #8790061 - Flags: review?(kmaglione+bmo) → review+

Comment 8

a year ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/314075c09505
Call tabs.create sooner r=kmag

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/314075c09505
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

11 months ago
See Also: → bug 1312492

Updated

9 months ago
Depends on: 1325830

Updated

9 months ago
Assignee: nobody → rob
You need to log in before you can comment on or make changes to this bug.