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

RESOLVED FIXED in Firefox 52

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

unspecified
mozilla52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: triaged)

Attachments

(1 attachment)

(Assignee)

Description

3 years 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

3 years 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

3 years ago
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Updated

3 years ago
Blocks: 1287007
See Also: 1287007
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

3 years ago
mozreview-review
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 7

3 years ago
mozreview-review
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

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

Comment 9

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

Updated

3 years ago
See Also: → 1312492

Updated

2 years ago
Depends on: 1325830

Updated

2 years ago
Assignee: nobody → rob

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.