Closed Bug 1366290 Opened 7 years ago Closed 7 years ago

Bad event ordering in webextension tabs API

Categories

(WebExtensions :: Untriaged, defect, P2)

53 Branch
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: blask, Assigned: bsilverberg)

Details

(Whiteboard: [tabs] triaged)

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170509210144

Steps to reproduce:

I am trying to port https://addons.mozilla.org/en-US/firefox/addon/tab-deque/ to webextension. It used to listen to TabOpen, TabSelect and TabClose and now I want to use their webextension equivalents. A minor problem is that the event order is not the same as in Chrome. A major problem is that onActivated(for the next tab) comes before onRemoved (in Chrome it's the other way around). Like this I can't tell whether it's the active tab or another tab that is being closed which is important for knowing which tab should get selected next. It would be great if the order could be changed. Alternatively, I could also work with more information being set on the onRemoved event...
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Flags: needinfo?(kmaglione+bmo)
Whiteboard: investigating
Assignee: nobody → bob.silverberg
Priority: -- → P5
Whiteboard: investigating → investigating[triaged][tabs]
I have verified this behaviour and have verified that Chrome behaves the opposite. Also, our internal TabSelect and TabClose events do fire in the same order as Chrome's API events - it is a quirk of the implementation that is causing them to fire in the wrong order, so this should be fixed.

The fix looks quite simple so I am going to produce a patch for it and attach it to this bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kmaglione+bmo)
Priority: P5 → P2
Whiteboard: investigating[triaged][tabs] → [tabs] triaged
Comment on attachment 8873106 [details]
Bug 1366290 - Fix the ordering of tabs.onActivated and tabs.onRemoved,

https://reviewboard.mozilla.org/r/144556/#review148620

I don't think that doing more of the `dispatchToMainThread()` hack is a good idea.
Off the top of my head, I think we should remove that hack for onRemoved and document that listeners are added asynchronously.  The comment that describes the situation sounds a lot like something that arises in a test, if that's the case the test can just use the same trick to defer the call to `window.close()`.  But I'm going to bounce this over to Kris, perhaps he'll come up with something even better.
Attachment #8873106 - Flags: review?(aswan)
Attachment #8873106 - Flags: review?(kmaglione+bmo)
Hi Bob and Kris, any ETA on reviewing/landing this?
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8873106 [details]
Bug 1366290 - Fix the ordering of tabs.onActivated and tabs.onRemoved,

https://reviewboard.mozilla.org/r/144556/#review172984

I'm pretty sure this is going to adversely affect the ordering of onActivated relative to other events. We can probably safely remove the dispatchToMainThread in emitRemoved at this point, since events are made asynchronous in other layers at this point.

That said, I do think it makes more sense to dispatch onActivated before onRemoved when an active tab is being removed, since that avoids leaving consumers of the API in an intermediate state where the tab they expect to be active no longer exists.
Attachment #8873106 - Flags: review?(kmaglione+bmo) → review-
I actually don't think it matters which one comes first, but the order should be properly documented and the same across browsers. As Chrome came first, I think Firefox should use the same order.
Thanks for the reply, Kris. As you suggested, I fixed this be removing the dispatchToMainThread from emitRemoved. Now the events fire in the same order as in Chrome. I have submitted a new patch and flagged you for review.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8873106 [details]
Bug 1366290 - Fix the ordering of tabs.onActivated and tabs.onRemoved,

https://reviewboard.mozilla.org/r/144556/#review182884
Attachment #8873106 - Flags: review?(kmaglione+bmo) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf7a1267d47b
Fix the ordering of tabs.onActivated and tabs.onRemoved, r=kmag
https://hg.mozilla.org/mozilla-central/rev/cf7a1267d47b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(bob.silverberg) → qe-verify-
(In reply to shivanand from comment #15)
> Is this the same bug? https://bugzilla.mozilla.org/show_bug.cgi?id=1396758

The 3rd comment over there says "This is a different issue from bug 1366290, ..." so presumably not.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.