1416087 - windows.create does not wait for tabs to be loaded

Status: REOPENED

(WebExtensions :: General, defect, P5)

Description

[Dietrich Ayala (:dietrich)]

56.0.2 (64-bit) Mac OS X

I get this error: "Error: Missing host permission for the tab" and in the code location column it says "undefined".

My permissions look like:

  "permissions": [
    "<all_urls>",
    "activeTab",
    "tabs"
  ]

(Just trying to get it to work, I shouldn't need anything but activeTab.)

The code is basically:

(async () => {

  let win = await browser.windows.create({
    url: "https://google.com"
  });

  browser.tabs.executeScript(win.tabs[0].id, {
    file: '/content-script.js'
  });

})();

Comment 1

[Kris Maglione [:kmag]]

Why would you not need anything other than "activeTab"? There's nothing in that code sample that would grant you the activeTab permission.

In any case, at the time when you call `executeScript`, the tab won't have begun loading yet. It will either be uninitialized or still contain an about:blank document with a null principal.

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #1)
> Why would you not need anything other than "activeTab"? There's nothing in
> that code sample that would grant you the activeTab permission.

You are correct. See comment #0, where I said exactly this.

> In any case, at the time when you call `executeScript`, the tab won't have
> begun loading yet. It will either be uninitialized or still contain an
> about:blank document with a null principal.

If that is true, it seems like a set-up-to-fail scenario for extension developers.

Seems like it should have *begun* loading. Why would it not have even begun to load? I just asked it to in window.create(), no?

Regardless, the docs say the default runAt value for executeScript is document_idle, which makes it sound like it wouldn't bother injecting the script until later when it's ok to do so.

Comment 3

[Dietrich Ayala (:dietrich)]

I keep reading through the docs, and seems like what I'm doing should be totally ok based on runAt defaulting to document_idle.

Testing Kris' theory, I set a timeout and wait 1s, and yep it works.

I changed to listen to onUpdated until tab id matches and status is 'complete', and now it works... sometimes (see below).

Really I shouldn't have to use onUpdated at all. I asked the window to open URL x, and the semantics of runAt set to 'document_idle' should work as expected from that point.

But as I said, even when waiting for `onUpdated(tab id matches && status==complete)`, I'm still getting the original reported error sometimes and my content script not loaded.

I also see other errors, only sometimes:

11:15:41.868 Error: Script 'moz-extension://4eb0da72-4b4e-7943-82bc-7ad2e5b375fe/content-script.js' result is non-structured-clonable data 1 content-script.js

11:18:02.918 Error: Message manager disconnected 1 undefined

Comment 4

[Dietrich Ayala (:dietrich)]

Testing against nightly (58) makes the unreliability go away.

I filed bug 1416108 for changing the structured clonable error into warning or just removing it.

Comment 5

[Andrew Swan [:aswan]]

The main issue here seems to be about the fact that there is a moment after a new tab is created (in this case it the tab in a newly created window), when it is at about:blank and it hasn't yet begun loading the requested URL.
That's not a behavior that I think we should change -- it would be a bunch of code to maintain and its easy for extensions to get the desired behavior as described above.  Additionally, it would be extra overhead for extensions that don't care about waiting for the initial load to start.

I do agree this could be documented better.  We already document essentially the same issue here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Tabs/onCreated

But please feel free to open a docs issue to document it in other relevant places like browser.tabs.create() and browser.windows.create()

Comment 6

[Dietrich Ayala (:dietrich)]

Please do not close bugs prematurely.

Drive-by dismissal is an effective way to stop people from participating - an open source anti-pattern.

> If that is true, it seems like a set-up-to-fail scenario for extension developers.

I would like to raise this concern up to the module owner for consideration.

The semantics of the API create an expectation by the developer that this scenario should be perfectly ok. Well, not just ok - it seems like it's exactly how it *should* work.

The ability to detect when developers are tripping on this problem is basically zero: They could, as a result just not be using the API, or just giving up trying to make their extension, and we would never know.

The reasons for not fixing this, as provided in comment #5, are a textbook example of putting the extension developer's needs *last*:

* To do it correctly is more work for us.

* To do it correctly means more code.

* Why do it correctly when the devs can probably, hopefully, maybe go find the workaround (and will also be motivated to do so, after first being frustrated by failure)?

* The initial overhead for extensions that don't use this *might* be a legitimate concern, but first would need to be investigated for cost, and if there is a non-negligible cost then the discussion is actually about the *trade-offs*.

Comment 7

[Andy McKay]

I've renamed the title to what I think accurately represents the request in comment 3.

There's nothing in the docs or code to suggest that when a new Window is created we'll wait for all the tabs to load and then resolve the promise. It's unclear to me why we would, can you explain that more other than "I thought that's what would happen"?

The windows API is about creating windows and we return the success to the add-on developer as fast as we can. Some may want to attach a content_script, others may not. We can't build an API that significantly hinders some use cases at the convenience of others - if we can help it.

I'll also note that Chrome and Edge do not exhibit the behaviour (as far as I can tell). Meaning that any deviation from Chrome or Edge on this would in fact disadvantage Firefox and all our developers. They would now have to work around the fact that Firefox is different.

The spec at: https://browserext.github.io/browserext/#dom-browserextwindows-create doesn't mention anything about waiting for the tabs loaded in the extension to complete either.

Based on what I think the issue is, I agree with aswan. If I've misunderstood, please let me know.

Comment 8

[Dietrich Ayala (:dietrich)]

I filed the issue because the code sample in comment #0 looks like it should work. "I thought that's what would happen" is exactly my point. We're of opposite minds, I guess. You say there's nothing to suggest it would work, and I'm saying there's nothing to suggest it wouldn't :)

Aside from that, I explained in the last bit of comment #2 and then in more detail in comment #3, after digging into the docs, why it seemed to me that it *should* actually work even if the tab isn't loaded:

The docs say the default runAt value for executeScript is document_idle, which makes it sound like it wouldn't bother injecting the script until later when it's ok to do so. I'm not saying to wait for tabs to load - that's something y'all brought into this. Based on the docs, it looks like the code in comment #0 should work even if the tab isn't loaded. The internal implementation detail of an about:blank loading, even though the caller has already specified the URL they'd like to be loaded, shouldn't be the caller's problem.

Feel free to close the issue if you don't think that makes sense, and thanks for looking into it.

Just so they're not lost, some things that might be worth following up:

* The code location being undefined in the error. It would really help developers to know where the problem is being triggered.

* The activeTab permission not working as expected. Kris confirmed that it should work that way in comment #1.

* Updating the documentation to be explicit instead of implicit in all places where the scenario might occur, as Andrew suggested in comment #5.

Comment 9

[Andrew Swan [:aswan]]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> * The activeTab permission not working as expected. Kris confirmed that it
> should work that way in comment #1.

Kris actually said the opposite.  Please see the documentation:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#activeTab_permission

Comment 11

[Dietrich Ayala (:dietrich)]

Relevant notes from the duplicate bug 1438672:

* Fixing this would achieve compat w/ Chrome's behavior. This means some Chrome add-ons will break without adding a workaround for Firefox.

* Same problem occurs when using browser.tabs.create().

Comment 12

[Rob Wu [:robwu]]

We should probably not delay the windows.create callback, but add some logic to tabs.executeScript as I sketched in at the bottom of: https://bugzilla.mozilla.org/show_bug.cgi?id=1418655#c8

Comment 13

[David Durst [:ddurst]]

(In reply to Rob Wu [:robwu] from comment #12)
> We should probably not delay the windows.create callback, but add some logic
> to tabs.executeScript as I sketched in at the bottom of:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1418655#c8

I'm fine with this approach.

I do want to point out with regard to "doing it correctly" that we're operating on an assumption: it's not illogical, but neither is it guaranteed. We maybe hope the devs can find a workaround for a lot more things than this, so it's certainly more about favouring our resource constraints than what we ask of developers to understand and incorporate.

Specifically, I disagree with this:

> The internal implementation detail of an about:blank loading, even though the caller has already specified the 
> URL they'd like to be loaded, shouldn't be the caller's problem.

I think developers should absolutely understand that a created window will open with a(n about:blank) tab opening before whatever they set. I don't consider this a fiddly detail, but rather a fundamental understanding of the differences between browser.window and browser.tabs and the events therein. The only bug here is that we don't make this so clear in the docs as to obviate the possible expectation[1].

That said, it does seem reasonable to accommodate this to make developers' lives easier -- especially as it's a default action on our part -- so I'm going to assign to Rob as P5[2].


[1] If we have not already documented this in browser.tabs.create() and browser.windows.create(), we should.
[2] It has sat this long (with apologies, we're digging through triage backlog), it isn't urgent, and I need Rob to finish some other things first. Rob can move this higher according to his workload expectations.

Comment 14

[Serade]

After 5 years, this issue still persists. Which shouldn't be called a bug, but rather an important feature.
In response to this comment :I'll also note that Chrome and Edge do not exhibit the behaviour (as far as I can tell). Meaning that any deviation from Chrome or Edge on this would in fact disadvantage Firefox and all our developers. They would now have to work around the fact that Firefox is different. I don't know how It was 5 years ago, but executeScript() works perfectly fine on chromium while the same code fails to load injected script on Firefox.
The only workaround I found I found was adding a setTimeout() to the code. If there's something I missed on the documentation or another workaround solution that gives better results, I'd deeply appreciate to see.

Best Regards!