586693 - Do we still need to marshal browser events?

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Ian Gilman [:iangilman]]

From dolske's review of bug 574217:

> >+          setTimeout(function() { // Marshal event from chrome thread to DOM thread
> 
> I'm not sure what that comment means. Everything is running on one thread.

Things may have changed since I started this pattern, but at the time (a few months ago), events about xul:tabs (such as TabSelect) were in fact arriving on a different thread than the main UI thread of the Tab Candy frame. This was readily apparent with some logging; two of our routines were running simultaneously in parallel. This caused all sorts of extremely unpleasant, unpredictable bugs. Since we've added those setTimeouts to event end points, all of those bugs have gone away. 

Might as well take a look and see if this is still necessary.

Comment 1

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 2

[:Gavin Sharp [email: [e-mail]]]

It's not possible in our embedding for two JS functions to be running "in parallel". It's possible to return to the event loop between invocations (using a timeout, e.g.), and even to spin the event loop in JS (using nsIThreadManager, if you're evil) and run nested JS code, but two pieces of JS code cannot run concurrently, so you must have been seeing something else.

Comment 3

[Ed Lee :Mardak]

Attachment: v1

I tested out removing the setTimeouts and seemed to cause the failing tests to pass when applying the patch from bug 574875. There were some broken tabcandy zooming, etc, but Raymond has fixed those up together in this patch.

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Comment 5

[Ed Lee :Mardak]

Removal of setTimeout fixes a number of bugs and beta blockers without leading to test failures: bug 587029 bug 586552 bug 574875

Comment 6

[(no longer active)]

This blocks bug 574875, which is a blocker.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 466255 [details] [diff] [review]
v1

Looks larger than it really is because of some format/indent changes, but setTimeout remove is righteous.

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/a15f73b8d431
* Removed some timeouts and fixed some broken user interactions that fixes various other bugs and test failures.
Bug 587029 - Tab Candy : closing last tab of a group leads to an isolated tab
Bug 586552 - GroupItem.newTab feedback should be immediate
* Init TabItems before handling firstrun tab grouping
* Removed _stopZoomPreparation related code since we are not using it anymore.
* Fixed the issue related to using move to other group feature. The moved tab is still visible in the tab bar after moving it to other group.