Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Basic tabs API support for Android WebExtensions

RESOLVED FIXED in Firefox 54
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Android
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: kmag, Assigned: kmag, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla54
Unspecified
Android
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [tabs]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments)

59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
5.45 KB, application/x-zip-compressed
Details
2.80 KB, text/plain
Details
(Assignee)

Description

a year ago
The first stage should include basic mapping of Fennec tabs to WebExtension tab objects, tagging of contexts with the correct tab IDs, and basic query support.
(Assignee)

Updated

a year ago
Blocks: 1260550

Updated

a year ago
OS: Unspecified → Android
Whiteboard: [tabs]
Blocks: 1260250

Updated

a year ago
Whiteboard: [tabs] → [tabs]triaged
Blocks: 1294390

Updated

11 months ago
Blocks: 1263005

Updated

11 months ago
Blocks: 467520

Updated

11 months ago
Blocks: 1300811

Updated

11 months ago
No longer blocks: 1263005

Updated

9 months ago
Component: WebExtensions: Untriaged → WebExtensions: Android
Priority: -- → P3

Updated

9 months ago
Blocks: 1226547

Updated

9 months ago
No longer blocks: 467520

Updated

7 months ago
webextensions: --- → +
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

6 months ago
mozreview-review
Comment on attachment 8831506 [details]
Bug 1260548: Part 1 - Factor out the common functionality of the tabs API.

https://reviewboard.mozilla.org/r/108052/#review109356

Whew, this is a big one but there are lots of excellent improvements here, thanks!
I also understand some of your comments on bug 1307348 a lot better now.
My only real feedback (and there's a line-item comment related to this below) is that we could really use some high-level documentation about all the pieces here (the `*Manager` and `*Tracker` classes for instance).  Reading their individual implementations is the best option right now, but that is more confusing than it needs to be since they user a bunch of existing components that require methods with specific names (observe/handleEvent/etc) but these methods are not distinguished from public methods and other internal methods.  I think we can both make those classes more readable (by clearly annotating individual methods) and also add documentation (though that can be separated from this bug)

::: browser/components/extensions/ext-tabs.js:463
(Diff revision 1)
>  
> -          return TabManager.convert(extension, tab);
> +          return tabManager.convert(tab);
>          });
>        },
>  
> -      remove: function(tabs) {
> +      async remove(tabs) {

Ooh, when did we get the green light to use async?

::: browser/components/extensions/ext-tabs.js:767
(Diff revision 1)
>            newTab.addEventListener("SSTabRestored", function listener() {
>              // Once it has been restored, select it and return the promise.
> -            newTab.removeEventListener("SSTabRestored", listener);
>              gBrowser.selectedTab = newTab;
> -            return resolve(TabManager.convert(extension, newTab));
> -          });
> +
> +            return resolve(tabManager.convert(newTab));

i don't think resolve() actually returns anything so the return here is pointless?

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js:24
(Diff revision 1)
>      // of removing the listener. This is to minimize any IPC, since the bug that
>      // is being tested is sensitive to timing.
>      let ignore = false;
>      let url;
>      browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
> -      if (changeInfo.status === "loading" && tab.url === url && !ignore) {
> +      if (url && changeInfo.status === "loading" && tab.url === url && !ignore) {

What changed that requires this?

::: toolkit/components/extensions/ExtensionTabs.jsm:67
(Diff revision 1)
> +    if (this.hasTabPermission) {
> +      return this._url;
> +    }
> +  }
> +
> +  get uri() {

So we have a uri property that is an nsIURI and a url property that is the string representation of the uri?  That feels clunky...

::: toolkit/components/extensions/ExtensionTabs.jsm:86
(Diff revision 1)
> +    }
> +  }
> +
> +  get favIconUrl() {
> +    if (this.hasTabPermission) {
> +      return this._favIconUrl;

Add a comment above that this is meant to be implemented in derived classes?
I notice in other places you put an implementation that throws an Error into the base class...
Same for other getters that are expected to be implemented in derived classes.

::: toolkit/components/extensions/ExtensionTabs.jsm:131
(Diff revision 1)
> +    if (this.hasTabPermission) {
> +      for (let prop of ["url", "title", "favIconUrl"]) {
> +        let val = this[`_${prop}`];
> +        if (val) {
> +          result[prop] = val;
> +        }
> +      }
> +    }

why not just use the uri/title/favIconUrl getters

::: toolkit/components/extensions/ExtensionTabs.jsm:323
(Diff revision 1)
> +
> +  get haveListeners() {
> +    return this._openListeners.size > 0 || this._closeListeners.size > 0;
> +  }
> +
> +  addOpenListener(listener) {

Just reading from here down, we have a mix of methods that are clearly part of the public API (eg addListener), callbacks that have to be named as they are to implement other interfaces (eg observer, handleEvent, etc) and other methods that are unclear to me (eg this one, addOpenListener).
A comment laying this all out or, ideally, grouping the private methods separately from the public ones, would help a lot I think.
Attachment #8831506 - Flags: review?(aswan) → review+
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1260250
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1294390
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1260550
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1301578
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1334891

Comment 23

6 months ago
mozreview-review
Comment on attachment 8831507 [details]
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic.

https://reviewboard.mozilla.org/r/108054/#review109402

::: browser/components/extensions/ext-tabs.js:125
(Diff revision 2)
>    awaitTabReady(tab) {
>      let deferred = this.tabReadyPromises.get(tab);
>      if (!deferred) {
>        deferred = PromiseUtils.defer();
> -      if (!this.initializingTabs.has(tab) && tab.linkedBrowser.innerWindowID) {
> +      if (!this.initializingTabs.has(tab) && (tab.linkedBrowser.innerWindowID ||
> +                                              tab.linkedBrowser.currentURI.spec === "about:blank")) {

how is this related to the rest of this patch?

::: browser/components/extensions/ext-tabs.js:148
(Diff revision 2)
>        return tabTracker.getTab(tabId);
>      }
>      return tabTracker.activeTab;
>    }
>  
> +  async function promiseTabOrActive(tabId) {

This name isn't very clear, add Ready to the end?

::: browser/components/extensions/ext-tabs.js:461
(Diff revision 2)
>  
>            if (createProperties.pinned) {
>              window.gBrowser.pinTab(tab);
>            }
>  
> -          if (createProperties.url && !createProperties.url.startsWith("about:")) {
> +          if (createProperties.url && createProperties.url !== window.BROWSER_NEW_TAB_URL) {

Again, I don't understand how this is related to the rest of this patch

::: browser/components/extensions/ext-tabs.js:623
(Diff revision 2)
>  
> -      executeScript: function(tabId, details) {
> -        return self.tabs._execute(tabId, details, "js", "executeScript");
> +      async insertCSS(tabId, details) {
> +        let tab = await promiseTabOrActive(tabId);
> -      },
>  
> -      insertCSS: function(tabId, details) {
> +        return tab.insertCSS(context, details);

I suspect I'm misunderstanding something here, but why isn't this `return await tab.insertCSS...`?

Comment 24

6 months ago
mozreview-review
Comment on attachment 8831508 [details]
Bug 1260548: Part 3 - Factor out the extension popup code into its own module.

https://reviewboard.mozilla.org/r/108056/#review109412
Attachment #8831508 - Flags: review?(aswan) → review+
(Assignee)

Comment 25

6 months ago
mozreview-review-reply
Comment on attachment 8831506 [details]
Bug 1260548: Part 1 - Factor out the common functionality of the tabs API.

https://reviewboard.mozilla.org/r/108052/#review109356

Yeah, I was planning to go through and add doc comments to everything. I just wanted to get the patches out as soon as possible to avoid as much rebase hell as I could.

> Ooh, when did we get the green light to use async?

I've always been aiming for 54. If any issues come up or someone objects, I'll fix them or take the fall. In any case, there's already a fair amount of usage in the tree.

> i don't think resolve() actually returns anything so the return here is pointless?

Oops. Yeah, I have no idea why that was there.

> What changed that requires this?

Nothing. The test was briefly failing because a bug was causing `tab.url` to be undefined, and the initial undefined value caused the handler to be executed early. The result was confusing, and I just wanted to avoid letting it happen again.

> So we have a uri property that is an nsIURI and a url property that is the string representation of the uri?  That feels clunky...

Yeah, I'm not especially happy about it either. The `url` property is there because that's the property name on the tab object we expose to content, and I want all of those properties to be consistent. The `uri` property is just kind of handy, but it also kind of bugs me.

> Add a comment above that this is meant to be implemented in derived classes?
> I notice in other places you put an implementation that throws an Error into the base class...
> Same for other getters that are expected to be implemented in derived classes.

Yeah, I should have added comments about these. The properties that are gated on permissions have the underscored values implemented in the derived class, and the gated getters implemented in the base class.

> why not just use the uri/title/favIconUrl getters

Just for the sake of performance. Those getters all check for tab permissions, and while that's not especially expensive with the current implementation, I'd still rather avoid the overhead.

I'll add a comment.

> Just reading from here down, we have a mix of methods that are clearly part of the public API (eg addListener), callbacks that have to be named as they are to implement other interfaces (eg observer, handleEvent, etc) and other methods that are unclear to me (eg this one, addOpenListener).
> A comment laying this all out or, ideally, grouping the private methods separately from the public ones, would help a lot I think.

Yeah, I was actually thinking about adding underscores to all of the private method names. I decided to stop waffling over it and just push, but it's probably a good idea.

Comment 26

6 months ago
mozreview-review
Comment on attachment 8831509 [details]
Bug 1260548: Part 4 - Factor out tab status listener logic into shared module.

https://reviewboard.mozilla.org/r/108058/#review109416
Attachment #8831509 - Flags: review?(aswan) → review+
(Assignee)

Comment 27

6 months ago
mozreview-review-reply
Comment on attachment 8831507 [details]
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic.

https://reviewboard.mozilla.org/r/108054/#review109402

> how is this related to the rest of this patch?

When I changed the `executeScript` calls to wait for the tab to load before validating arguments, one of the tests that checked invalid arguments failed, because the promise never resolved for the tab it opened with an explicit "about:blank" URL.

> Again, I don't understand how this is related to the rest of this patch

Same issue as the other change. This code path is only necessary for the new tab URL. Taking it for about:blank breaks things, but the tests were previously missing the issue.

> I suspect I'm misunderstanding something here, but why isn't this `return await tab.insertCSS...`?

Because `return await` is Considered Harmful. Returning a promise has exactly the same effect as returning its resolution value.

Comment 28

6 months ago
mozreview-review
Comment on attachment 8831510 [details]
Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules.

https://reviewboard.mozilla.org/r/108060/#review109422

::: browser/components/extensions/ext-utils.js:336
(Diff revision 2)
>      }, Ci.nsIThread.DISPATCH_NORMAL);
>    }
>  
> -  getBrowserId(browser) {
> +  getBrowserData(browser) {
> +    if (!browser.ownerGlobal.location.href === "about:addons") {
> +      // When we're loaded into a <browser> inside about:addons, we need to go up

I know this is just moving existing code but what's the scenario that this code applies to?  inline options pages?

::: browser/components/extensions/ext-utils.js:341
(Diff revision 2)
> +      // When we're loaded into a <browser> inside about:addons, we need to go up
> +      // one more level.
> +      browser = browser.ownerGlobal.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIDocShell)
> +                       .chromeEventHandler;
> +    }

the original code bailed out here if this returned null, that's no longer necessary?
Attachment #8831510 - Flags: review?(aswan) → review+
(Assignee)

Comment 29

6 months ago
mozreview-review-reply
Comment on attachment 8831510 [details]
Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules.

https://reviewboard.mozilla.org/r/108060/#review109422

> I know this is just moving existing code but what's the scenario that this code applies to?  inline options pages?

Yeah, just inline options pages.

> the original code bailed out here if this returned null, that's no longer necessary?

Right, it's no longer necessary now that we explicitly check that the parent document is about:addons. That will always have a parent <browser> element.

Comment 30

6 months ago
mozreview-review
Comment on attachment 8831507 [details]
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic.

https://reviewboard.mozilla.org/r/108054/#review109430

The handling of about:blank and about:newtab here feels really awkward but you know the details here much better than I do so I trust if there was a practical way to do this more neatly that you would have found it...
Attachment #8831507 - Flags: review?(aswan) → review+
(Assignee)

Comment 31

6 months ago
mozreview-review-reply
Comment on attachment 8831507 [details]
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic.

https://reviewboard.mozilla.org/r/108054/#review109430

Basically, the problem is bug 1315553. Modifying top-level `about:blank` currently isn't supported because of the way that it's loaded. Fixing that bug should make this workaround unnecessary. But, in the mean time, the workaround prevents these calls from simply stalling until the first time another URL is loaded into the tab.

Comment 32

6 months ago
mozreview-review
Comment on attachment 8831511 [details]
Bug 1260548: Part 6 - Add basic tabs API support for Android.

https://reviewboard.mozilla.org/r/108062/#review109438

Wow, this is impressive.
My only criticism here is the inconsistent use of async and directly returning Promises in ext-tabs.js but I'd be fine leaving cleanup of that for a follow-up.

::: mobile/android/components/extensions/ext-tabs.js:341
(Diff revision 2)
>          for (let tabId of tabs) {
>            let tab = tabTracker.getTab(tabId);
> -          tab.ownerGlobal.gBrowser.removeTab(tab);
> +          tab.browser.ownerGlobal.BrowserApp.closeTab(tab);
>          }
> +
> +        return Promise.resolve();

why not just make this async?

::: mobile/android/components/extensions/ext-tabs.js:366
(Diff revision 2)
> -            tabbrowser.selectedTab = tab;
> +            BrowserApp.selectTab(tab);
>            } else {
>              // Not sure what to do here? Which tab should we select?
>            }
>          }
> -        if (updateProperties.muted !== null) {
> +        // FIXME: highlighted/selected, muted, pinned, openerTabId

follow-up bug?  (unless these are in one of the subsequent patches that i haven't seen yet)

::: mobile/android/components/extensions/ext-tabs.js:368
(Diff revision 2)
> -            tabbrowser.unpinTab(tab);
> -          }
> -        }
> -        // FIXME: highlighted/selected, openerTabId
>  
> -        return tabManager.convert(tab);
> +        return Promise.resolve(tabManager.convert(tab));

Why the `Promise.resolve` ?

::: toolkit/components/extensions/ExtensionParent.jsm:213
(Diff revision 2)
>      // tabs.sendMessage / tabs.connect
>      if (tabId) {
>        // `tabId` being set implies that the tabs API is supported, so we don't
>        // need to check whether `tabTracker` exists.
>        let tab = apiManager.global.tabTracker.getTab(tabId, null);
> -      return tab && tab.linkedBrowser.messageManager;
> +      return tab && (tab.linkedBrowser || tab.browser).messageManager;

Ugh.
how about something like `const BROWSER_PROPERTY = (desktop) ? "linkedBrowser" : "browser";` at the top level and then just `tab[BROWSER_PROPERTY]` here?
Attachment #8831511 - Flags: review?(aswan) → review+
(Assignee)

Comment 33

6 months ago
mozreview-review-reply
Comment on attachment 8831511 [details]
Bug 1260548: Part 6 - Add basic tabs API support for Android.

https://reviewboard.mozilla.org/r/108062/#review109438

> why not just make this async?

Rebase botch.

> follow-up bug?  (unless these are in one of the subsequent patches that i haven't seen yet)

Well, muted and pinned are not currently supported on Android. `openerTabId` already has a bug for desktop, and I was planning to handle both at once. I still need to file follow-ups for the other missing functionality, including some of these properties.

> Why the `Promise.resolve` ?

Also merge botch.

> Ugh.
> how about something like `const BROWSER_PROPERTY = (desktop) ? "linkedBrowser" : "browser";` at the top level and then just `tab[BROWSER_PROPERTY]` here?

I was considering cleaner ways to handle this, and was leaning toward either adding some method to tabTracker to handle this, or figuring out some way to use a Tab instance here. I'd rather do it in a follow-up, though, since this is an unusual use case.

Comment 34

6 months ago
mozreview-review
Comment on attachment 8831512 [details]
Bug 1260548: Part 7 - Add mochitests for the Android tabs API.

https://reviewboard.mozilla.org/r/108064/#review109452

This looks good.  I assume you've given though to possibly sharing some of this code and didn't do it since it wasn't practical to do now?  Is it worth a follow-up bug to do something at some point to avoid having so much copied test code?

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript.html:186
(Diff revision 2)
>            browser.test.assertEq("http://example.com/", result[0], "Script executed correctly in new tab");
>  
>            await browser.tabs.remove(tab.id);
>          }),
>  
> +        // This currently does not work on Android.

either remove it or have a follow-up bug?
Attachment #8831512 - Flags: review?(aswan) → review+

Comment 35

6 months ago
mozreview-review
Comment on attachment 8831513 [details]
Bug 1260548: Part 8 - Cut down on unnecessary console warnings during test runs.

https://reviewboard.mozilla.org/r/108066/#review109464

Nice, thanks.
Attachment #8831513 - Flags: review?(aswan) → review+
(Assignee)

Comment 36

6 months ago
mozreview-review-reply
Comment on attachment 8831512 [details]
Bug 1260548: Part 7 - Add mochitests for the Android tabs API.

https://reviewboard.mozilla.org/r/108064/#review109452

Yeah, I was planning to move as much as possible to toolkit, and then delete the corresponding browser tests, once Android tab support is more complete. I'll file a bug for that, once I've filed its dependencies.
(Assignee)

Comment 37

6 months ago
Currently missing from the Android tabs API:

The following methods and events:

- detectLanguage
- getZoom
- getZoomSettings
- move
- onMoved
- onZoomChange
- setZoom
- setZoomSettings

In tabs.create:

- cookieStoreId
- pinned
- ownerTabId

In tabs.update:

- muted
- pinned
Keywords: dev-doc-needed
(Assignee)

Comment 38

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e70a396920869369a36eebbcf307835ce806ee5
Bug 1260548: Part 1 - Factor out the common functionality of the tabs API. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/70f6eb26604a29da53fc8fd64d098d885aeac366
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/628d13328225f0f004a8ade3d3a3d266df5b5351
Bug 1260548: Part 3 - Factor out the extension popup code into its own module. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac255bc85d767e26424eb7182b0eb7bbe16ca51
Bug 1260548: Part 4 - Factor out tab status listener logic into shared module. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/980266a5316b3bb73844aa17a50a7121ffd9de1d
Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/683a0eb722b663c43b90cd83aa23e99e82288efa
Bug 1260548: Part 6 - Add basic tabs API support for Android. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8f0e831975e3f24c0b2c518134a8bc4e93e1e4
Bug 1260548: Part 7 - Add mochitests for the Android tabs API. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e67a3e9a5bfd7eff6327963a9c66fa13f3d7e80
Bug 1260548: Part 8 - Cut down on unnecessary console warnings during test runs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc59eff957ebf8d522b91f275ec3838e50df3392
Bug 1260548: Part 9 - Make sure Android mochitests do not leave extra tabs open. r=aswan
(Assignee)

Comment 39

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca8ceb7b7fabca08691e54930acf604d3aee960
Bug 1260548: Follow-up: Add missing support file to chrome.ini. r=me
(Assignee)

Comment 40

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e250509dc1d3267e22e694383e19d881b09adc9c
Bug 1260548: Follow-up: Fix inadequate skip-if. r=me
(Assignee)

Comment 41

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/40833d595677539789acc9bc55e45f18b2c614e3
Bug 1260548: Follow-up: Fix skip-if typo that was the real problem. r=me CLOSED TREE
(Assignee)

Comment 42

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d1a9455d3cdee5668117c3ffea9345568192c1
Bug 1260548: Follow-up: Add another missing dependency to mochitest.ini. r=me
(Assignee)

Comment 43

6 months ago
One thing I have learned about Android mochitests is that no matter how many try runs and local test runs I do, I will never win...
https://hg.mozilla.org/mozilla-central/rev/9e70a3969208
https://hg.mozilla.org/mozilla-central/rev/70f6eb26604a
https://hg.mozilla.org/mozilla-central/rev/628d13328225
https://hg.mozilla.org/mozilla-central/rev/7ac255bc85d7
https://hg.mozilla.org/mozilla-central/rev/980266a5316b
https://hg.mozilla.org/mozilla-central/rev/683a0eb722b6
https://hg.mozilla.org/mozilla-central/rev/1c8f0e831975
https://hg.mozilla.org/mozilla-central/rev/8e67a3e9a5bf
https://hg.mozilla.org/mozilla-central/rev/bc59eff957eb
https://hg.mozilla.org/mozilla-central/rev/1ca8ceb7b7fa
https://hg.mozilla.org/mozilla-central/rev/e250509dc1d3
https://hg.mozilla.org/mozilla-central/rev/40833d595677
https://hg.mozilla.org/mozilla-central/rev/04d1a9455d3c
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

6 months ago
Blocks: 1336308
Depends on: 1159532
Depends on: 1271190

Comment 45

5 months ago
We have tested this bug with “every event 1.0” add-on recommended by andym and Krupa.

Purpose of test: see that the implemented APIs are fired and works correctly.

Results:

1.No issues were observed on the mobile device regarding Firefox browser

2.During the tests, the console recorded multiple issues that involved the implemented APIs (tabs, webRequest , webNavigation) and they were extracted in separate txt files for further investigation by the dev side.

3.Two clear patterns were identified which led to issues observed in the console:

- google search fires “browser.webRequest.onResponseStarted” API and returns “NS_BINDING_ABORTED: Component returned failure code: 0x804b0002”

- closing a tab fires “browser.tabs.onHighlighted” and returns "TypeError: this.browser.webProgress is undefined”

4.For the other issues we don’t have a clear pattern (yet). Basically they appear when navigating.

Please let us know your thoughts and how can we be of help anymore. We will attach the txt files with errors.

Comment 46

5 months ago
Created attachment 8836081 [details]
Errors from console during testing
(Assignee)

Comment 47

5 months ago
Sorry for the delay. I started responding to this and forgot to finish.

(In reply to Victor Carciu from comment #45)
> - google search fires “browser.webRequest.onResponseStarted” API and returns
> “NS_BINDING_ABORTED: Component returned failure code: 0x804b0002”

This is normal. It's a side-effect of the way error reporting works in certain
corner cases, but it is expected.

> - closing a tab fires “browser.tabs.onHighlighted” and returns "TypeError:
> this.browser.webProgress is undefined”

It looks like this is happening because we're firing an event after the tab's
<browser> element has been removed. Can you please file a bug with steps to
reproduce?

> 4.For the other issues we don’t have a clear pattern (yet). Basically they
> appear when navigating.

> TypeError: this._recipeManager is null[Learn More]

This is an issue with the login manager. I've seen it before, and it probably
merits a bug, but it's unrelated to these changes.

> RangeError: too many arguments provided for a function call SessionStore.js:1154:17

This may be related, but it's hard to say without a complete stack, and
knowing what's on that line in the build that this came from.

> NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]  browser.js:4106

This is probably unrelated, but probably deserves its own Android bug with
steps to reproduce.

> TypeError: can't access dead object  tab.js:1656:1

This looks like it may be a devtools bug, but is probably unrelated.

Comment 48

5 months ago
For "> TypeError: can't access dead object" I have opened the source and from what I see, it's indeed related to devtools (attached txt file "viewsource for...")

Comment 49

5 months ago
Created attachment 8839115 [details]
Viewsource for TypeError can't access dead object.txt
I've updated the compat data according to comment 37, I think: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs#Browser_compatibility. But please let me know if I need anything else here.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1358415

Updated

a month ago
See Also: → bug 1372178
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.