Do we still need to marshal browser events?

RESOLVED FIXED in Firefox 4.0b4

Status

RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: iangilman, Assigned: raymondlee)

Tracking

Trunk
Firefox 4.0b4
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

v1
14.79 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Depends on: 574217

Updated

8 years ago
Whiteboard: b5

Comment 1

8 years ago
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Component: TabCandy → TabCandy
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.

Updated

8 years ago
Assignee: nobody → raymond

Comment 3

8 years ago
Created attachment 466248 [details] [diff] [review]
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.
Attachment #466248 - Flags: review?(dolske)
Attachment #466248 - Flags: feedback?(ian)

Updated

8 years ago
Attachment #466248 - Attachment is obsolete: true
Attachment #466248 - Flags: review?(dolske)
Attachment #466248 - Flags: feedback?(ian)

Comment 4

8 years ago
Created attachment 466255 [details] [diff] [review]
v1
Attachment #466255 - Flags: review?(dolske)
Attachment #466255 - Flags: feedback?(ian)

Updated

8 years ago
Blocks: 587029

Comment 5

8 years ago
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

Comment 6

8 years ago
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

Updated

8 years ago
Attachment #466255 - Flags: approval2.0?

Updated

8 years ago
Attachment #466255 - Flags: approval2.0?

Comment 9

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4

Updated

8 years ago
Blocks: 586992

Updated

8 years ago
Blocks: 586861

Updated

8 years ago
Depends on: 587922
(Assignee)

Updated

8 years ago
Blocks: 587062

Updated

8 years ago
Blocks: 587163
(Assignee)

Updated

8 years ago
Blocks: 586689
(Assignee)

Updated

8 years ago
Blocks: 586551

Updated

8 years ago
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.