Closed Bug 1213975 Opened 5 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Whiteboard: [tabs])

Attachments

(4 files, 12 obsolete files)

1.52 KB, patch
rpl
: review+
Details | Diff | Splinter Review
7.89 KB, patch
rpl
: review+
Details | Diff | Splinter Review
1.44 KB, patch
rpl
: review+
Details | Diff | Splinter Review
2.47 KB, patch
rpl
: review+
Details | Diff | Splinter Review
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.
Attached patch fix.patch (obsolete) — Splinter Review
This attached patch contains the small fix on the ext-tabs.js api module
Attachment #8672763 - Flags: review?(wmccloskey)
Attached patch test-case.patch (obsolete) — Splinter Review
This patch contains a new related test case.
Attachment #8672764 - Flags: review?(wmccloskey)
Whiteboard: [tabs]
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.
Attachment #8672763 - Flags: review?(wmccloskey) → review+
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.
Attachment #8672764 - Flags: review?(wmccloskey) → review+
Assignee: nobody → luca.greco
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.
Attachment #8674248 - Flags: review?(wmccloskey)
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).
Attachment #8674249 - Flags: review?(wmccloskey)
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.
Attachment #8674250 - Flags: review?(wmccloskey)
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 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!
Attachment #8674248 - Flags: review?(wmccloskey) → review+
Attachment #8674249 - Flags: review?(wmccloskey) → review+
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.
Attachment #8674250 - Flags: review?(wmccloskey)
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).
Attachment #8674250 - Attachment is obsolete: true
Attachment #8674804 - Flags: review?(wmccloskey)
Attachment #8674804 - Flags: review?(wmccloskey) → review+
Blocks: webext
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
Attachment #8674248 - Attachment is obsolete: true
Attachment #8675301 - Flags: review?(wmccloskey)
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)
Attachment #8674247 - Attachment is obsolete: true
Attachment #8675699 - Flags: review+
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.
Attachment #8675301 - Attachment is obsolete: true
Attachment #8675301 - Flags: review?(wmccloskey)
Attachment #8675701 - Flags: review?(wmccloskey)
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)
Attachment #8674249 - Attachment is obsolete: true
Attachment #8675702 - Flags: review+
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)
Attachment #8674804 - Attachment is obsolete: true
Attachment #8675703 - Flags: review+
Attachment #8675701 - Flags: review?(wmccloskey) → review+
Flags: blocking-webextensions+
Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675699 - Attachment is obsolete: true
Attachment #8678792 - Flags: review+
Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675701 - Attachment is obsolete: true
Attachment #8678793 - Flags: review+
Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675702 - Attachment is obsolete: true
Attachment #8678794 - Flags: review+
Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675703 - Attachment is obsolete: true
Attachment #8678795 - Flags: review+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.