1562844 - [geckoview] WebExtension tabs and webNavigation event listeners support

Status: RESOLVED FIXED

(GeckoView :: Extensions, defect, P3)

Description

[Krzysztof Jan Modras [:chrmod]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

In GeckoView WebExtension APIs are not working correctly as their implementation comes from Fennec and it relies on it's internals.

Specifically tab.onCreated, tabs.onRemoved, tabs.onUpdated, webNavigation.onCommited throw errors when listeners are assigned.

Comment 1

[Krzysztof Jan Modras [:chrmod]]

Attachment: Bug 1562844 - GeckoView webextensions tabs and webnavigation listeners support

This changes provide basic support for webextenion tabs and webNavigation listeners by implementing missing objects on which Fennec implementation was relying.

Comment 3

[Krzysztof Jan Modras [:chrmod]]

I have repeatedly run the tests, and they often failed on TV. These failures are unrelated to this patch, because the patch only affects GeckoView code, while TV only runs on Fennec due to bug 1566423.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a9a7371947a
GeckoView webextensions tabs and webnavigation listeners support r=robwu,rpl,snorp

Comment 5

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2a9a7371947a9557fe4fd32641bd8fd555b11968&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Cmochitests%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-mochitest-e10s-3%2Cm%283%29&selectedJob=261128372

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261128372&repo=autoland&lineNumber=6903

Backout link: https://hg.mozilla.org/integration/autoland/rev/3bd2aca8271901b2318b02a1a435e61a7764e770

[task 2019-08-12T16:05:35.083Z] 16:05:35 INFO - 3146 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_onUpdated.html
[task 2019-08-12T16:05:35.083Z] 16:05:35 INFO - Buffered messages logged at 16:05:25
[task 2019-08-12T16:05:35.083Z] 16:05:35 INFO - 3147 INFO add_task | Entering test test_onUpdated
[task 2019-08-12T16:05:35.083Z] 16:05:35 INFO - 3148 INFO Extension loaded
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - Buffered messages finished
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - 3149 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_tabs_onUpdated.html | got unexpected number of updateInfo data - Expected: [{"status":"complete","url":"about:blank"},{"status":"loading"},{"status":"loading","url":"http://mochi.test:8888/tests/mobile/android/components/extensions/test/mochitest/context_tabs_onUpdated_page.html"},{"status":"complete"}], Actual: [{"status":"loading"},{"status":"loading","url":"http://mochi.test:8888/tests/mobile/android/components/extensions/test/mochitest/context_tabs_onUpdated_page.html"},{"status":"complete"}]
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:18
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - testHandler@SimpleTest/ExtensionTestUtils.js:59:18
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - testResult@SimpleTest/ExtensionTestUtils.js:73:18
[task 2019-08-12T16:05:35.084Z] 16:05:35 INFO - 3150 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_onUpdated.html | tabs.onUpdated

Comment 6

[Narcis Beleuzu [:NarcisB]]

There were also Raptor perma failures:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261154045&repo=autoland&lineNumber=1395

Comment 7

[Krzysztof Jan Modras [:chrmod]]

There was a change on central that homogenized the behavior around about:blank loading between Fennec and GeckoView.
After rebase with central I've fixed the failing test.

Not sure if Raptor failures are related. Can I do something to verify it locally?

Comment 8

[Krzysztof Jan Modras [:chrmod]]

Cancelling the checkin-needed. As Raptor failures are caused by this patch.

Comment 9

[Rob Wu [:robwu]]

The raptor test failure is because this patch fixed the WebExtension API implementation on GeckoView.
Previously, tabs.update(tabId, {url}) changed the URL of the foreground tab (regardless of the value of tabId). The patch for this bug fixed the implementation, and now tabs.update(tabId, ...) will look up the right tab before trying to navigate it.
Raptor uses tabId 0 by default, which is an invalid tabId.

• tabs.remove - https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/testing/raptor/webext/raptor/runner.js#645 (properly implemented in bug 1565782, causes non-fatal error message in console; regression of bug 1499067).
• tabs.update - https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/testing/raptor/webext/raptor/runner.js#459 (properly implemented in this bug, breaks Raptor)

If the objective is to update (and remove) the active foreground tab, then testTabId = null should be used instead of 0.
If you aren't sure, then null is the right value to use for tabs.update, since it results in the equivalent behavior before this patch.

Raptor on GeckoView used to not close the tab, because tabs.remove(0) has never closed any tab on GeckoView. Should we continue to not close the active tab (and get "Error: Invalid tab ID 0" (examlple)?

Comment 10

[Krzysztof Jan Modras [:chrmod]]

Attachment: Bug 1562844 - Use correct tabId in Raptor

More info: https://bugzilla.mozilla.org/show_bug.cgi?id=1562844#c9

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not sure why I was set as needinfo in comment 9. All questions regarding Raptor should actually go to Rob.

Comment 12

[Robert Wood [:rwood]]

(In reply to Rob Wu [:robwu] from comment #9)

The raptor test failure is because this patch fixed the WebExtension API implementation on GeckoView.
Previously, tabs.update(tabId, {url}) changed the URL of the foreground tab (regardless of the value of tabId). The patch for this bug fixed the implementation, and now tabs.update(tabId, ...) will look up the right tab before trying to navigate it.
Hi, comments inline thanks :)

Raptor uses tabId 0 by default, which is an invalid tabId.

• tabs.remove - https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/testing/raptor/webext/raptor/runner.js#645 (properly implemented in bug 1565782, causes non-fatal error message in console; regression of bug 1499067).
• tabs.update - https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/testing/raptor/webext/raptor/runner.js#459 (properly implemented in this bug, breaks Raptor)

If the objective is to update (and remove) the active foreground tab, then testTabId = null should be used instead of 0.
If you aren't sure, then null is the right value to use for tabs.update, since it results in the equivalent behavior before this patch.

Right so on geckoview browsers (GVE, refbrow, fenix) we needed to default because tabs.create wasn't supported at the time (and the callback to that receives a valid tabId for the newly created tab). So my understanding now is we should change that default to 'null' instead of 0 (I see in the patch, cool). Will that work on non-geckoview also (i.e. fennec, and Firefox desktop)? Is tabs.create supported now on geckoview? In which case instead of using a default (i.e. null) then we can grab the actual tabID at creation time.

Raptor runs on Firefox (and Chrome/ium) desktop as well as all the Firefox android platforms so any changes here we must ensure work on all the platforms, or change the runner.js to handle appropriately. So try runs will need to run at least one Raptor tp6 job on Firefox deskop, Chromium, fennec, refbrow, fenix, and GVE. Thanks!

Raptor on GeckoView used to not close the tab, because tabs.remove(0) has never closed any tab on GeckoView. Should we continue to not close the active tab (and get "Error: Invalid tab ID 0" (examlple)?

Is tabs.remove supported on geckoview now? Can we provide 'null' here also for the tabID so it defaults to closing the current / foreground tab?

Thanks! :)

Comment 13

[Rob Wu [:robwu]]

(In reply to Robert Wood [:rwood] from comment #12)

Right so on geckoview browsers (GVE, refbrow, fenix) we needed to default because tabs.create wasn't supported at the time (and the callback to that receives a valid tabId for the newly created tab). So my understanding now is we should change that default to 'null' instead of 0 (I see in the patch, cool). Will that work on non-geckoview also (i.e. fennec, and Firefox desktop)?

Yes. The documented behavior is that if tabId is not set (=null), that the active tab is used.
Any correct WebExtension / Chrome extension API implementation will reject a value of "0".

Is tabs.create supported now on geckoview? In which case instead of using a default (i.e. null) then we can grab the actual tabID at creation time.

Creating new tabs with tabs.create is supported, provided that a WebExtensionController.TabDelegate, e.g. as done in TestRunnerActivity.java. To not block these patches any further, I suggest to open a new bug about using tabs.create on GeckoView in Raptor.

Raptor on GeckoView used to not close the tab, because tabs.remove(0) has never closed any tab on GeckoView. Should we continue to not close the active tab (and get "Error: Invalid tab ID 0" (examlple)?

Is tabs.remove supported on geckoview now? Can we provide 'null' here also for the tabID so it defaults to closing the current / foreground tab?

tabs.remove is supported now, but as Raptor doesn't seem to open tabs on GeckoView now, I think that not closing anything is the right behavior, for now.

Comment 14

[Robert Wood [:rwood]]

(In reply to Rob Wu [:robwu] from comment #13)

Yes. The documented behavior is that if tabId is not set (=null), that the active tab is used.
Any correct WebExtension / Chrome extension API implementation will reject a value of "0".

Ok. On the non-geckoview platforms Raptor is using the tabID from tabs.create and not zero so that's why it's working there.

Is tabs.create supported now on geckoview? In which case instead of using a default (i.e. null) then we can grab the actual tabID at creation time.

Creating new tabs with tabs.create is supported, provided that a WebExtensionController.TabDelegate, e.g. as done in TestRunnerActivity.java. To not block these patches any further, I suggest to open a new bug about using tabs.create on GeckoView in Raptor.

No it's ok - we are soon moving away from the Raptor web extension anyway, in favour of browsertime.

Raptor on GeckoView used to not close the tab, because tabs.remove(0) has never closed any tab on GeckoView. Should we continue to not close the active tab (and get "Error: Invalid tab ID 0" (examlple)?

Is tabs.remove supported on geckoview now? Can we provide 'null' here also for the tabID so it defaults to closing the current / foreground tab?

tabs.remove is supported now, but as Raptor doesn't seem to open tabs on GeckoView now, I think that not closing anything is the right behavior, for now.

Ah, makes sense, yes we can just leave it as/is.

Comment 15

[Krzysztof Jan Modras [:chrmod]]

:robwu did run the try to verify all is fine with raport https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3f4456083680af19c18bc7bd659121d8823bccc

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d74231becedc
GeckoView webextensions tabs and webnavigation listeners support r=robwu,rpl,snorp
https://hg.mozilla.org/integration/autoland/rev/5448f3aaa09a
Use correct tabId in Raptor r=robwu,rwood

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d74231becedc
https://hg.mozilla.org/mozilla-central/rev/5448f3aaa09a

Comment 18

[Chris Peterson [:cpeterson]]

Moving some WebExtension bugs to the GeckoView::Extensions component.