1213975 - tabs.onUpdated updatedInfo about location url changes should contain the status attribute

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

During tests on running an existent Chrome addon as a Firefox webextension, I noticed that we do not have the status attribute when we send updates about the location url, on the contrary on Chrome it has the status attribute.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: fix.patch

This attached patch contains the small fix on the ext-tabs.js api module

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-case.patch

This patch contains a new related test case.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8672763 [details] [diff] [review]
fix.patch

Review of attachment 8672763 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-tabs.js
@@ +193,5 @@
>            onLocationChange(browser, webProgress, request, locationURI, flags) {
>              let gBrowser = browser.ownerDocument.defaultView.gBrowser;
>              let tab = gBrowser.getTabForBrowser(browser);
>              let tabId = TabManager.getId(tab);
> +            let [needed, changeInfo] = sanitize(extension, {

While reading this over, I realized that we probably also want a check that says:

if (!webProgress.isTopLevel) {
  return;
}

Otherwise loads within frames will trigger this event, which we don't want. Please feel free to include this in your patch.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8672764 [details] [diff] [review]
test-case.patch

Review of attachment 8672764 [details] [diff] [review]:
-----------------------------------------------------------------

If it's not too much trouble, it would be cool if you could use a page that has an iframe in it instead of about:robots. Then we could test the isTopLevel thing as well.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
@@ +11,5 @@
> +    background: function() {
> +      var updatedInfoList = [];
> +
> +      browser.tabs.query({ active: true }, function ([tab]) {
> +

No need for the blank line here.

@@ +13,5 @@
> +
> +      browser.tabs.query({ active: true }, function ([tab]) {
> +
> +        browser.tabs.onUpdated.addListener(function (tabId, updatedInfo, tab) {
> +

Same here.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1213975-WebExtension-tab-update-events-about-loc.patch

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8674248 [details] [diff] [review]
0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

This patch contains the updated test case which has an iframe inside the test page.

The test scenario makes use of postMessage and browser.runtime.sendMessage to synchronize the code running into the various contexts without using hacky workarounds based on setTimeouts or setIntervals.

It fails consistently without the fix in the "0003" patch and passes with the fix applied.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8674249 [details] [diff] [review]
0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch

This patch apply the fix suggested in the previous comments which filter out url location updates related to non-top windows (e.g. iframes).

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8674250 [details] [diff] [review]
0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

This patch contains a fix to correctly unregister listeners installed by tabs.onUpdated's EventManager when the context is closed.

I caught this by trying to test the test case stability using "./mach mochitest --repeat 10 ...".

I added this fix into this bugzilla issue because it ensures the stability of the test case on multiple runs.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8674250 [details] [diff] [review]
0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

Review of attachment 8674250 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-tabs.js
@@ +215,5 @@
> +            AllWindowEvents.removeListener("TabPinned", listener);
> +            AllWindowEvents.removeListener("TabUnpinned", listener);
> +          }
> +        };
> +        context.callOnClose(onClose);

This part should not be necessary. All EventManager instances should be unregistered when the context is closed:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#130
Could you remove this and see if there's still a problem? Maybe the ext-utils.js thing was the only problem?

::: browser/components/extensions/ext-utils.js
@@ +275,5 @@
>      }
>  
>      let listeners = this._listeners.get(type);
>      listeners.delete(listener);
> +    if (listeners.size == 0) {

Thanks for finding this!

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8674248 [details] [diff] [review]
0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

Review of attachment 8674248 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8674250 [details] [diff] [review]
0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

Please ask for review again if it turns out we need the onClose code.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

This is the updated patch to fix "tabs.onUpdated cleanups on shutdown issues".

The "callOnClose" is not necessary so I removed it from this patch and fixed the optional manual unsubscribe closure returned from the "tabs.onUpdated" API method (which calls some "AllWindowEvents.addListener" instead of "AllWindowEvents.removeListener" by mistake).

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

I'm updating the patch which contains the new test case, because it fails during the try run:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=377b32581e84
- https://treeherder.mozilla.org/logviewer.html#?job_id=12684728&repo=try

It fails because it browser.tabs.onUpdated catches a '{ status: "completed" }' before the expected updates sequence.

I've slightly reworked the tests: now it creates a new window, which is closed when the test exits.

I've just launched a new try run with the updated testcase:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=20d928dd64f4

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1213975-WebExtension-tab-update-events-about-loc.patch

patch rebased on the updated mozilla-central tip
(it hasn't real changes besides the "index sha1..sha1 mode" line in the patch file so I'm re-appling the r+ flag)

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

patch rebased on the updated mozilla-central tip with resolved conflict on "browser/components/extensions/test/browser/browser.ini".

it contains the same changes on the test case of the previous version so I'm re-applying the r? flag on it.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch

patch rebased on the updated mozilla-central tip
(it hasn't real changes besides the "index sha1..sha1 mode" line in the patch file so I'm re-applying the r+ flag)

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

patch rebased on the updated mozilla-central tip
(it hasn't real changes besides the "index sha1..sha1 mode" line in the patch file so I'm re-applying the r+ flag)

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1213975-WebExtension-tab-update-events-about-loc.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0f0b978ecd3c
https://hg.mozilla.org/integration/fx-team/rev/144c402556a8
https://hg.mozilla.org/integration/fx-team/rev/b3a33728ece5
https://hg.mozilla.org/integration/fx-team/rev/148a40aae385

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f0b978ecd3c
https://hg.mozilla.org/mozilla-central/rev/144c402556a8
https://hg.mozilla.org/mozilla-central/rev/b3a33728ece5
https://hg.mozilla.org/mozilla-central/rev/148a40aae385