651311 - Race condition in TabView._initFrame()

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[ithinc]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Raymond Lee [:raymondlee]]

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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 527267 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=ed77e28c0543
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=bdb40a65a90f

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 527831 [details] [diff] [review]
patch v3

Lovely, thanks!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In my queue.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/5bb9cd5c939a