Race condition in TabView._initFrame()

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ithinc, Assigned: ttaubert)

Tracking

Trunk
Firefox 6

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110419 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110419 Firefox/6.0a1

See the following steps.

Reproducible: Always

Steps to Reproduce:
1. Restart Firefox and ensure Panorama is not initialized
2. Execute the following in JS shell:
TabView.moveTabTo(gBrowser.mTabs[1], null);
TabView.moveTabTo(gBrowser.mTabs[2], gBrowser.mTabs[1]._tabViewTabItem.parent.id);

Actual Results:  
Only gBrowser.mTabs[1] is moved to new group. Moving gBrowser.mTabs[2] fails.

Expected Results:  
Both tabs are moved to the new group.
(Reporter)

Updated

6 years ago
Blocks: 603789
Assignee: nobody → tim.taubert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Moving multiple tabs sequentially to new group fails → Race condition in TabView._initFrame()
Created attachment 527228 [details] [diff] [review]
patch v1
Attachment #527228 - Flags: feedback?(raymond)
Comment on attachment 527228 [details] [diff] [review]
patch v1

Looks fine. 

+  _initFrame: (function () {
+    let isLoading = false;
+    let callbacks = [];

However, it might be better to follow the coding convention i.e. make isLoading callback as variables of TabView and remove the anonymous function.
Attachment #527228 - Flags: feedback?(raymond) → feedback+
Created attachment 527267 [details] [diff] [review]
patch v2

(In reply to comment #2)
> However, it might be better to follow the coding convention i.e. make isLoading
> callback as variables of TabView and remove the anonymous function.

True, fixed.
Attachment #527228 - Attachment is obsolete: true
Attachment #527267 - Flags: review?(ian)
Comment on attachment 527267 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=ed77e28c0543
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=bdb40a65a90f
Comment on attachment 527267 [details] [diff] [review]
patch v2

>   _initFrame: function TabView__initFrame(callback) {
>-    if (this._window) {
>-      if (typeof callback == "function")
>+    if (typeof callback == "function") {
>+      if (this._window) {
>         callback();
>-    } else {
... 
>+        return;
>       }
> 
>-      this._setBrowserKeyHandlers();
>+      this._initFrameCallbacks.push(callback);
>     }
>+
>+    if (this._isFrameLoading)
>+      return;
>+
>+    this._isFrameLoading = true;

This new logic doesn't account for the possibility that this._window exists but no callback is used; please fix.

Otherwise looking good.
Attachment #527267 - Flags: review?(ian) → review-
Created attachment 527831 [details] [diff] [review]
patch v3
Attachment #527267 - Attachment is obsolete: true
Attachment #527831 - Flags: review?(ian)
Comment on attachment 527831 [details] [diff] [review]
patch v3

Lovely, thanks!
Attachment #527831 - Flags: review?(ian) → review+
Created attachment 527864 [details] [diff] [review]
patch for checkin
Attachment #527831 - Attachment is obsolete: true
Keywords: checkin-needed
In my queue.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5bb9cd5c939a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
No longer blocks: 603789
Blocks: 653099
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.