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

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla44
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [tabs])

Attachments

(4 attachments, 12 obsolete attachments)

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
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8672763 [details] [diff] [review]
fix.patch

This attached patch contains the small fix on the ext-tabs.js api module
Attachment #8672763 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

2 years ago
Created attachment 8672764 [details] [diff] [review]
test-case.patch

This patch contains a new related test case.
Attachment #8672764 - Flags: review?(wmccloskey)

Updated

2 years ago
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)

Updated

2 years ago
Assignee: nobody → luca.greco
(Assignee)

Comment 5

2 years ago
Created attachment 8674247 [details] [diff] [review]
0001-Bug-1213975-WebExtension-tab-update-events-about-loc.patch
Attachment #8672763 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8674248 [details] [diff] [review]
0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch
Attachment #8672764 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Created attachment 8674249 [details] [diff] [review]
0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch
(Assignee)

Comment 8

2 years ago
Created attachment 8674250 [details] [diff] [review]
0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch
(Assignee)

Comment 9

2 years ago
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)
(Assignee)

Comment 10

2 years ago
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)
(Assignee)

Comment 11

2 years ago
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!
Attachment #8674247 - Flags: review+
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)
(Assignee)

Comment 15

2 years ago
Created attachment 8674804 [details] [diff] [review]
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).
Attachment #8674250 - Attachment is obsolete: true
Attachment #8674804 - Flags: review?(wmccloskey)
Attachment #8674804 - Flags: review?(wmccloskey) → review+

Updated

2 years ago
Blocks: 1214433
(Assignee)

Comment 16

2 years ago
Created attachment 8675301 [details] [diff] [review]
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
Attachment #8674248 - Attachment is obsolete: true
Attachment #8675301 - Flags: review?(wmccloskey)
(Assignee)

Comment 17

2 years ago
Created attachment 8675699 [details] [diff] [review]
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)
Attachment #8674247 - Attachment is obsolete: true
Attachment #8675699 - Flags: review+
(Assignee)

Comment 18

2 years ago
Created attachment 8675701 [details] [diff] [review]
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.
Attachment #8675301 - Attachment is obsolete: true
Attachment #8675301 - Flags: review?(wmccloskey)
Attachment #8675701 - Flags: review?(wmccloskey)
(Assignee)

Comment 19

2 years ago
Created attachment 8675702 [details] [diff] [review]
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)
Attachment #8674249 - Attachment is obsolete: true
Attachment #8675702 - Flags: review+
(Assignee)

Comment 20

2 years ago
Created attachment 8675703 [details] [diff] [review]
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)
Attachment #8674804 - Attachment is obsolete: true
Attachment #8675703 - Flags: review+
Attachment #8675701 - Flags: review?(wmccloskey) → review+

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 21

2 years ago
Created attachment 8678792 [details] [diff] [review]
0001-Bug-1213975-WebExtension-tab-update-events-about-loc.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675699 - Attachment is obsolete: true
Attachment #8678792 - Flags: review+
(Assignee)

Comment 22

2 years ago
Created attachment 8678793 [details] [diff] [review]
0002-Bug-1213975-add-WebExtension-tabs.onUpdated-test-cas.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675701 - Attachment is obsolete: true
Attachment #8678793 - Flags: review+
(Assignee)

Comment 23

2 years ago
Created attachment 8678794 [details] [diff] [review]
0003-Bug-1213975-filter-out-from-WebExtension-tabs.onUpda.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675702 - Attachment is obsolete: true
Attachment #8678794 - Flags: review+
(Assignee)

Comment 24

2 years ago
Created attachment 8678795 [details] [diff] [review]
0004-Bug-1213975-fix-WebExtension-tabs.onUpdated-cleanup-.patch

Updated patch, rebased on a recent mozilla-central and re-applied r+.
Attachment #8675703 - Attachment is obsolete: true
Attachment #8678795 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
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
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.