1223254 - Figure out when tabs API callbacks should fire, and make them fire at the right time

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[Kris Maglione [:kmag]]

The Chrome API docs, unfortunately, do not generally document when callbacks are meant to fire. For the tabs API, most callbacks currently fire almost immediately, which is almost certainly too early.

In e10s, after calling tabs.update, for instance, the callback should almost certainly not fire until the equivalent onUpdated event.

For the following functions, I'm less clear on when we'd expect the callbacks to fire in e10s windows:

* create
* remove
* reload
* executeScript
* insertCss

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Keep in mind that anything run through runSafe happens in the next turn of the event loop. Extensions seem to depend on this. Also, I think onUpdated will fire before the tabs.update callback, although I haven't tested this.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #1)
> Keep in mind that anything run through runSafe happens in the next turn of
> the event loop. Extensions seem to depend on this.

Right, but I'm pretty sure most of these callbacks are supposed to fire after some specific point. The executeScript one, at least, is documented, and is supposed to fire after the script has executed in the content tab. Most of the others don't actually say when they're supposed to fire, though.

> Also, I think onUpdated will fire before the tabs.update callback, although I haven't tested this.

Unfortunately, it doesn't. Not in e10s, anyway. I wrote a test that depended on it, and it failed, because it fired much earlier than onUpdated.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #2)
> > Also, I think onUpdated will fire before the tabs.update callback, although I haven't tested this.
> 
> Unfortunately, it doesn't. Not in e10s, anyway. I wrote a test that depended
> on it, and it failed, because it fired much earlier than onUpdated.

Which properties were you changing? The _tabAttrModified event seems to be sent right away, so I would expect that we queue the runnable for that update before the callback.

I guess if you change the URL then we'll certainly fire the callback for that before onUpdated. But I would be surprised if Chrome did otherwise.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #3)
> I guess if you change the URL then we'll certainly fire the callback for
> that before onUpdated. But I would be surprised if Chrome did otherwise.

I was updating the URL, yeah. I just tested in Chrome. The callback does fire before onUpdated, but it fires after the URL has been updated. Or, at least, the new URL is in the object passed to the callback, and `tabs.get` returns a tab object with the new URL.

I'm not entirely sure what that means about the internal state of anything. It does seem that Chrome gets it just as wrong as we do where it matters, for my use case, though. If you update a pageAction from the update callback, it still gets immediately reset unless you wait for onUpdated instead.

Comment 5

[Kris Maglione [:kmag]]

The main issues here have been fixed.