Closed Bug 586693 Opened 11 years ago Closed 11 years ago

Do we still need to marshal browser events?

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(blocking2.0 beta4+)

RESOLVED FIXED
Firefox 4.0b4
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 574217
Whiteboard: b5
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
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.
Assignee: nobody → raymond
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #466248 - Flags: review?(dolske)
Attachment #466248 - Flags: feedback?(ian)
Attachment #466248 - Attachment is obsolete: true
Attachment #466248 - Flags: review?(dolske)
Attachment #466248 - Flags: feedback?(ian)
Attached patch v1Splinter Review
Attachment #466255 - Flags: review?(dolske)
Attachment #466255 - Flags: feedback?(ian)
Blocks: 587029
Removal of setTimeout fixes a number of bugs and beta blockers without leading to test failures: bug 587029 bug 586552 bug 574875
Blocks: 586552, 574875
Status: NEW → ASSIGNED
This blocks bug 574875, which is a blocker.
blocking2.0: --- → ?
blocking2.0: ? → beta4+
Whiteboard: b5
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.
Attachment #466255 - Flags: review?(dolske) → review+
Duplicate of this bug: 587029
Attachment #466255 - Flags: approval2.0?
Attachment #466255 - Flags: approval2.0?
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
Blocks: 586992
Blocks: 586861
Depends on: 587922
Blocks: 587062
Blocks: 587163
Blocks: 586689
Blocks: 586551
Attachment #466255 - Flags: feedback?(ian)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.