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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Rob Wu [:robwu]]

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

Comment 1

[Rob Wu [:robwu]]

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 2

[Rob Wu [:robwu]]

Attachment: Bug 1301862 - Call tabs.create sooner

Call tabs.create immediately after the tab is created without delay to
reduce the chance of losing tabs.onUpdated events.
To fulfill the needs of bugzil.la/1254003 (which introduced the delay),
I wait until the `innerWindowID` is set before sending the message down
in the APIs that depend on `innerWindowID`. The missing `innerWindowID`
is what caused bug 1254003, so this should be fine.

I manually confirmed that the `innerWindowID` is `null` initially for
http-URLs, moz-extension-URLs, about:blank (with and without e10s) and
eventually set (according to the source code, browser._innerWindowID is
set before the onLocationChange notification is sent for the top-level
frame). The original test case from bug 1254003 passes even with this
change.

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

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

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

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

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

Comment 5

[Kris Maglione [:kmag]]

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.

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

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

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8790061 [details]
Bug 1301862 - Call tabs.create sooner

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/314075c09505
Call tabs.create sooner r=kmag

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/314075c09505