Closed
Bug 1301862
Opened 8 years ago
Closed 8 years ago
WebExtension tests should not rely on tabs.onUpdated inside a callback
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: robwu, Assigned: robwu)
References
Details
(Whiteboard: triaged)
Attachments
(1 file)
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•8 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•8 years ago
|
Priority: -- → P3
Whiteboard: triaged
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•8 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•8 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+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/314075c09505
Call tabs.create sooner r=kmag
Comment 9•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Assignee: nobody → rob
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Assignee | ||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•