Last Comment Bug 651311 - Race condition in TabView._initFrame()
: Race condition in TabView._initFrame()
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 6
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: 653099
  Show dependency treegraph
 
Reported: 2011-04-19 17:40 PDT by ithinc
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (7.63 KB, patch)
2011-04-20 04:22 PDT, Tim Taubert [:ttaubert]
raymond: feedback+
Details | Diff | Review
patch v2 (8.37 KB, patch)
2011-04-20 07:59 PDT, Tim Taubert [:ttaubert]
ian: review-
Details | Diff | Review
patch v3 (8.34 KB, patch)
2011-04-22 12:17 PDT, Tim Taubert [:ttaubert]
ian: review+
Details | Diff | Review
patch for checkin (6.37 KB, patch)
2011-04-22 14:45 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review

Description ithinc 2011-04-19 17:40:03 PDT
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] 2011-04-20 04:22:09 PDT
Created attachment 527228 [details] [diff] [review]
patch v1
Comment 2 Raymond Lee [:raymondlee] 2011-04-20 05:47:25 PDT
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] 2011-04-20 07:59:44 PDT
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.
Comment 5 Ian Gilman [:iangilman] 2011-04-22 09:54:55 PDT
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] 2011-04-22 12:17:57 PDT
Created attachment 527831 [details] [diff] [review]
patch v3
Comment 7 Ian Gilman [:iangilman] 2011-04-22 14:37:24 PDT
Comment on attachment 527831 [details] [diff] [review]
patch v3

Lovely, thanks!
Comment 8 Tim Taubert [:ttaubert] 2011-04-22 14:45:40 PDT
Created attachment 527864 [details] [diff] [review]
patch for checkin
Comment 9 :Ms2ger 2011-04-23 02:39:35 PDT
In my queue.

Note You need to log in before you can comment on or make changes to this bug.