626455 - modal dialog in onbeforeunload: browser freeze after removing last tab group in panorama

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Stefan]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre

Resembles Bug 626451

Reproducible: Always

Steps to Reproduce:
1. prepare clean state
2. open "about:home" in tab 1
3. open testcase oba.htm in tab 2
4. switch into panorama (ctrl-e)
5. close tabgroup (click at "X")
6. click at "X" right of "Undo Close Group"
7. try to switch back out of panorama (ctrl-e)
8. close the new tab
Actual Results:  
7-stays in panorama
8-freeze

Expected Results:  
6-close browser.

Comment 1

[Stefan]

Attachment: testcase uba.htm

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Is this a dupe of bug 613800?

Comment 3

[Stefan]

Don't know. Seems related.

Comment 5

[Raymond Lee [:raymondlee]]

When the chain (GroupItem_closeHidden() -> TabItem_close() =>gBrowser.remove() => _beginRemoveTab() => permitUnload()) is invoked, the beforeunload event is fired. However, permitUnload() doesn't return true/falas until user dismisses the alert() defined in the beforeunload method on the webpage.  This prevents the subsequence tabview code in the TabItem_close() and GroupItem_closeHidden() to be executed.  I wonder whether a new event or something can be created and fired inside permitUnload() so tabview know when to exit the tabview UI and focus the tab with alert() executed in the beforeunload method.

Comment 6

[Dão Gottwald [:dao]]

Raymond, you're doing it again. :/

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Raymond, you're doing it again. :/

Sorry, I didn't change that intentionally.

Comment 8

[Mihai Sucan [:msucan]]

I spent the day analyzing this bug.

Problem description:

Once you close the "undo group close" bubble, Panorama really closes the tabs.  The first time you close a group, it is only hidden (group.closeAll()). The click handler for the X button that allows you to permanently remove a hidden group invokes closeHidden().

Execution anatomy:

- group.closeHidden()
  - create a new empty group with an empty tab, if needed, so firefox doesn't close itself.
  - group.destroy()
    - tabitem.close() for each tab in the group
      - tabbrowser.removeTab(tab) for each tab in the group
        - tabbrowser._beginRemoveTab()
       - tabbrowser._endRemoveTab()
    - group.close()

Things work fine, but the STR in comment 0 complicates things quite a lot.

- _beginRemoveTab() execution gets stuck in the call:

    if (ds && ds.contentViewer && !ds.contentViewer.permitUnload())

This is a synchronous call which waits for the user input, to allow or not, page unload. The onbeforeunload tabprompt is shown and the user is asked by Firefox to allow or not leaving the page.

So, all execution is halted in _beginRemoveTab() - which means the tabview group is left in an undefined state (it's hidden, and about to destroy itself).

The onbeforeunload tabprompt is shown and it fires DOMWillOpenModalDialog. The event handler in tabbrowser focuses the tab of the prompt.

The TabView UI.onTabSelect() fires and determines that it Panorama is visible, and it needs to give focus to the said tab. The UI.hideTabView() method is called.

The hideTabView() method calls GroupItems.removeHiddenGroups() which prunes all groups that are closed (hidden). It finds the group of the currently selected tab (which is showing a tabprompt), the group that is about to be entirely destroyed. The group is removed once again: group.closeHidden(). This time no new empty group is added, but destroy() is called again. This time tabbrowser._beginRemoveTab() breaks, and it fails when it tries to the remove the tab with tbe prompt - browser.webProgess is undefined, also in _endRemoveTab() the property browser.destroy() is not a function.

In the meantime, on screen the user ends up seeing the new empty tab created in the empty group, or he stays in Panorama. Depending which code executes first.

Execution also depends on: was the last selectedTab the tab where the prompt shows? If yes, the DOMWillOpenModalDialog won't call TabView.UI.onTabSelect(). This look fine, everything seems as if it worked, but next time when you try to close a tab, tabbrowser breaks badly.

The ultimate result is that _endRemoveTab() is called in a loop, with its execution ending in the following check:

  if (!aTab || !aTab._endRemoveArgs) return;

aTab._endRemoveArgs evaluates to false.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Proposed fix. The work I did in bug 613800 made me really interested into fixing this bug as well - this is because the symptoms and problems encountered here and there are similar and inter-connected.

Solution description:

- in TabView.UI.onTabSelect() if TabView is visible, and if the newly selected tab is in a hidden group... we unhide the group. This is to prevent group destruction, to prevent GroupItems.removeHiddenGroup() from trying to destroy the same group again, when hideTabView() calls that method.

- in TabView.GroupItem.closeHidden() we change the call to newTab() to create a new tab but not zoom into it. The newTab() method is also updated to allow such behavior. This is needed to avoid the conflict between DOMWillOpenModalDialog which tries to focus the tab with the prompt ... and this call which also tries to select the new tab. With this change, we stay in Panorama when the last group is closed, and just have the new group + empty tab shown - instead of zooming into it.

(this should not be considered a behavior regression. we cannot know before-hand if the tab removal attempts will display prompts or not.)

- in tabbrowser, the DOMWillOpenModalDialog event handler makes sure that TabView.UI.onTabSelect() is called, even for the currently selectedTab. This is needed because the onTabSelect event handler is not called when the currently selected tab is the same as the tab that opens the prompt. If we don't call onTabSelect here, Panorama is not hidden, and the group close is not undone, and everything still remains stuck.

The result of applying the patch: when the user tries to close the group, the tab with the prompt is focused, and the user is given the choice to leave the page (or not). If he chooses to leave the page, the entire group is closed, and the new empty tab is focused. If he chooses no, the page stays in focus and the group is not closed - it is unhidden.

Comments are welcome. Thanks!

(please note that the patch depends on the patch from bug 613800)

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 511136 [details] [diff] [review]
proposed fix

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -2534,19 +2534,27 @@
>           }
>         ]]>
>       </handler>
>       <handler event="DOMWillOpenModalDialog" phase="capturing">
>         <![CDATA[
>           if (!event.isTrusted)
>             return;
> 
>+          let previousTab = this.selectedTab;
>+
>           // We're about to open a modal dialog, make sure the opening
>           // tab is brought to the front.
>           this.selectedTab = this._getTabForContentWindow(event.target.top);
>+
>+          if (previousTab == this.selectedTab &&
>+              window.TabView && TabView.isVisible()) {
>+            let win = TabView.getContentWindow();
>+            win.UI.onTabSelect(this.selectedTab);
>+          }

This needs a better solution. Nothing guarantees that calling onTabSelect for an already-selected tab won't break stuff or just do nothing at all.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> Comment on attachment 511136 [details] [diff] [review]
> proposed fix
> 
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> >--- a/browser/base/content/tabbrowser.xml
> >+++ b/browser/base/content/tabbrowser.xml
> >@@ -2534,19 +2534,27 @@
> >           }
> >         ]]>
> >       </handler>
> >       <handler event="DOMWillOpenModalDialog" phase="capturing">
> >         <![CDATA[
> >           if (!event.isTrusted)
> >             return;
> > 
> >+          let previousTab = this.selectedTab;
> >+
> >           // We're about to open a modal dialog, make sure the opening
> >           // tab is brought to the front.
> >           this.selectedTab = this._getTabForContentWindow(event.target.top);
> >+
> >+          if (previousTab == this.selectedTab &&
> >+              window.TabView && TabView.isVisible()) {
> >+            let win = TabView.getContentWindow();
> >+            win.UI.onTabSelect(this.selectedTab);
> >+          }
> 
> This needs a better solution. Nothing guarantees that calling onTabSelect for
> an already-selected tab won't break stuff or just do nothing at all.

Agreed, but I still haven't found a better way to do it. Hence, let's see what Ian suggests we can do.

Comment 12

[Ian Gilman [:iangilman]]

(In reply to comment #11)
> Agreed, but I still haven't found a better way to do it. Hence, let's see what
> Ian suggests we can do.

What if you catch the DOMWillOpenModalDialog event in Tab View and deal with it there?

Comment 13

[Mihai Sucan [:msucan]]

(In reply to comment #12)
> (In reply to comment #11)
> > Agreed, but I still haven't found a better way to do it. Hence, let's see what
> > Ian suggests we can do.
> 
> What if you catch the DOMWillOpenModalDialog event in Tab View and deal with it
> there?

I thought of that, but it would serve a very specific purpose. It would kick into action only if newTab = selectedTab, and it would duplicate a part of the onTabSelect() code (that is ... hideTabView if isVisible, and unhide group if needed, etc).

Is that acceptable?

Comment 14

[Ian Gilman [:iangilman]]

(In reply to comment #13)
> I thought of that, but it would serve a very specific purpose. It would kick
> into action only if newTab = selectedTab, and it would duplicate a part of the
> onTabSelect() code (that is ... hideTabView if isVisible, and unhide group if
> needed, etc).
> 
> Is that acceptable?

Yes, I assume you would check for that condition. It could then call onTabSelect directly, or you could refactor so that this handler and onTabSelect both call the same common routine.

Comment 15

[Raymond Lee [:raymondlee]]

Assigning to msucan since he has some ideas how to fix this.

Comment 16

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch.

Changes:

- no longer touching tabbrowser, and no longer relying on the patch from bug 613800.

- added a DOMWillOpenModalDialog event handler to TabView UI. This calls onTabSelect() when the tab that shows the modal dialog is the same as the selected tab - knowing that onTabSelect() is not called for such cases.

I did not move the shared code to a different method, I just call onTabSelect because I learned I don't need just to hide the TabView, or just unhide the tab group. I also need to properly set the active group (which onTabSelect does). So, pretty much all the code is relevant.

- made the GroupItem.closeHidden() to zoom into the new empty tab, if the group is really closed. This is needed to not cause test failures.

- the new empty tab is closed when the group is not closed, because if the user chooses to stay on page, he'll end up with a new empty group with an empty tab (when he switches back to Panorama).

- updated the test for bug 599626 to avoid failures. Now the groupShown event is fired earlier (when the modal dialog shows up, as explained in comment 9). Similarly, tabviewhidden is fired earlier as well.

No test failures on my system.

Looking forward to your feedback! Thanks! If things look fine, then I can add a mochitest as well.

Comment 17

[Ian Gilman [:iangilman]]

Comment on attachment 511779 [details] [diff] [review]
updated patch

Looking good! Comments: 

>+      this.onDOMWillOpenModalDialog = this.onDOMWillOpenModalDialog.bind(this);
>+      gWindow.addEventListener("DOMWillOpenModalDialog",
>+        this.onDOMWillOpenModalDialog, false);

Please also removeEventListener on shutdown. The idiom we use is:

    this._cleanupFunctions.push(function() {
      gWindow.removeEventListener("DOMWillOpenModalDialog", self.onDOMWillOpenModalDialog, false);
    });

>+  // Function: getTabForContentWindow
>+  // Find a tab given by the content window.
>+  // Parameters:
>+  //   event - the DOMWillOpenModalDialog event object.
>+  // Returns the XUL tab element or null.
>+  getTabForContentWindow: function UI_getTabForContentWindow(window) {

That parameter comment is incorrect; please fix.

I look forward to seeing the test!

Comment 18

[Ian Gilman [:iangilman]]

Comment on attachment 511779 [details] [diff] [review]
updated patch

One more thing: 

>   // Function: newTab
>   // Creates a new tab within this groupItem.
>-  newTab: function GroupItem_newTab(url) {
>+  // Returns the new tab XUL element.
>+  newTab: function GroupItem_newTab(url, dontZoomIn) {

The idiom we prefer is to pass an optional options object that would have an optional dontZoomIn property. This makes calling code self-documenting, and scales better. Can you do that here? Also, please document the new argument in the comments for the function.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated patch. Thanks for the feedback+ Ian!

Made the changes you requested and added a mochitest which always reproduces the hang using the STR of comment 0. Things run as expected when the patch is applied, of course no hangs. :)

Looking forward to your review. Thanks!

Comment 20

[Ian Gilman [:iangilman]]

Comment on attachment 513424 [details] [diff] [review]
patch update 2

Looks good! Has it passed try?

One thing: please use the public domain license for your new test. 

Also, for future reference, there are a number of handy helper routines in head.js. No need to refactor this test to use them, though.

Comment 21

[Mihai Sucan [:msucan]]

(In reply to comment #20)
> Comment on attachment 513424 [details] [diff] [review]
> patch update 2
> 
> Looks good! Has it passed try?

Will push the patch to the try server.

> One thing: please use the public domain license for your new test. 

Will change that.

> Also, for future reference, there are a number of handy helper routines in
> head.js. No need to refactor this test to use them, though.

Yeah, I took Raymond's test and modified it to suit the new STR. ;)

Thanks for the review+!

Comment 22

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Rebased and updated the test license. Will push this patch to the try server now.

Comment 23

[Mihai Sucan [:msucan]]

Try server builds:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-56a83371c5cf

Try server results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=56a83371c5cf


They look good to me, but please confirm. There are failures, but as they look, they are the usual noise, unrelated test failures.

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 513578 [details] [diff] [review]
patch update 3

Asking for approval2.0+: this patch fixes a Firefox hang that is caused by pages that show alerts/prompts when the last tabs group in Panorama is closed.

The patch has review+, and the try server results are linked in comment 23.

Thanks!

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513578 [details] [diff] [review]
patch update 3

a=beltzner

Comment 26

[Mihai Sucan [:msucan]]

Thanks for the approval, Mike!

Comment 27

[Mihai Sucan [:msucan]]

hm, looks like the patch needs rebasing. will attach updated patch.

Comment 28

[Mihai Sucan [:msucan]]

Attachment: debug patch

There seems to be a problem.

Applied the patch, which still applies cleanly, but now the browser_tabview_bug599626.js test file fails.

Did a bit of investigation ... it looks like there's a regression (?) which this patch exposes now.

In testStayOnPage() the tabviewhidden event is fired, then TabView.toggle() is called, then the tabviewshown event is fired, but Panorama is still not visible, hence things fail.

If I add the taviewhidden event listener before the call to setupAndRun() in testStayOnPage() I can see that the tabviewhidden event is fired twice in a row. So, again, this shows there's something wrong going on.

Looked into the TabView UI code and I have a hunch what's going on there: TabView.showTabView() and hideTabView() both use isTabViewVisible() which returns true/false based on a *very* specific condition that, I believe, does not hold true *while* Panorama is showing/hiding. So, consecutive calls from multiple places to hideTabView() execute because isTabViewVisible() does not *yet* return true. (Similar things happen probably for consecutive calls to showTabView() from multiple places as well.)

Perhaps we should keep track of Panorama hiding/showing with a _isHiding/_isShowing flag, to avoid problems.

Thoughts?

Comment 29

[Kevin Hanes]

Comment on attachment 514294 [details] [diff] [review]
debug patch

Ian's out for a week, moving feedback over to mitcho

Comment 30

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated the patch to fix the test failure.

As expected it's a problem with concurrent calls to hideTabView(), which is called once when the tab is given focus to show the modal dialog, and the second time when the test tries to zoom into a different tab.

I added a flag that fixes the problem, but the test also needed updates. Please let me know if the changes are fine. Thanks!

Comment 31

[Mihai Sucan [:msucan]]

Attachment: patch update 4 (changes only)

Adding a diff that shows the changes from patch update 3 - given this is your first look at the patch.

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

[bugspam: betaN -> final]

has patch, got approval during beta12, but apparently there's an issue? We'll see if this can be re-approved.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513578 [details] [diff] [review]
patch update 3

(removing approval due to subsequent comments)

Comment 34

[Ian Gilman [:iangilman]]

Comment on attachment 514552 [details] [diff] [review]
patch update 4 (changes only)

Tim, you've done some work in this area recently, right? Can you take a look at the approach?

I guess I'm concerned about swallowing commands when we're in transition. On the other hand, what would the right response be anyway? Maybe we should assert and make sure no one calls us in that fashion?

At any rate, it would be great to get this modal dialog bug fixed if we can… it's a nasty situation for the user.

Comment 35

[Mihai Sucan [:msucan]]

Indeed, the patch is not ready, but I am currently waiting for feedback before I can make any further changes.

Comment 36

[Kevin Hanes]

bugspam

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514552 [details] [diff] [review]
patch update 4 (changes only)

Review of attachment 514552 [details] [diff] [review]:
-----------------------------------------------------------------

Are the changes to browser_tabview_bug599626.js necessary? The test passes locally without those changes. browser_tabview_bug612470.js has been updated in the meantime and passes, too.

I'm really sorry that this took so long, it just got lost somehow :/ Mihai, do you still want to be assigned to this?

::: browser/base/content/tabview/ui.js
@@ +438,3 @@
>        return;
>  
> +    this._isChangingVisibility = true;

Nit: please initialize this property at the top of the file and add a nice comment.

@@ +512,5 @@
>  
>        TabItems.resumePainting();
>      }
> +
> +    this._isChangingVisibility = false;

Please do that right before the dispatchEvent() calls in both branches (when animating and when not).

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514548 [details] [diff] [review]
patch update 4

Review of attachment 514548 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks good. There are some nits in the test which should be cleaned up. Thanks for the patch, Mihai!

::: browser/base/content/tabview/groupitems.js
@@ +784,5 @@
>      }
>  
> +    let closed = this.destroy();
> +
> +    if (!closed && tab) {

Please return early here "if (!tab)". So we don't need to check "if (... && tab)" for every branch and we can have a cleaner "if (closed) { ... } else { ... }" structure.

::: browser/base/content/test/tabview/browser_tabview_bug626455.js
@@ +12,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  window.addEventListener("tabviewshown", onTabViewWindowLoaded, false);
> +  TabView.toggle();

Please use showTabView(onTabViewWindowLoaded) here.

@@ +16,5 @@
> +  TabView.toggle();
> +}
> +
> +function onTabViewWindowLoaded() {
> +  window.removeEventListener("tabviewshown", onTabViewWindowLoaded, false);

(can be removed)

@@ +18,5 @@
> +
> +function onTabViewWindowLoaded() {
> +  window.removeEventListener("tabviewshown", onTabViewWindowLoaded, false);
> +
> +  let contentWindow = document.getElementById("tab-view").contentWindow;

Please use TabView.getContentWindow().

@@ +42,5 @@
> +    // stay on page
> +    doc.documentElement.getButton("cancel").click();
> +
> +    let onTabViewShown = function() {
> +      window.removeEventListener("tabviewshown", onTabViewShown, false);

(can be removed)

@@ +63,5 @@
> +      // start the next test
> +      testLeavePage(contentWindow, activeGroup);
> +    };
> +
> +    window.addEventListener("tabviewshown", onTabViewShown, false);

(can be removed)

@@ +66,5 @@
> +
> +    window.addEventListener("tabviewshown", onTabViewShown, false);
> +
> +    executeSoon(function() {
> +      TabView.toggle();

showTabView(onTabViewShown);

@@ +79,5 @@
> +  let endGame = function() {
> +    window.removeEventListener("tabviewhidden", endGame, false);
> +    tabViewHidden = true;
> +    if (groupClosed && dialogsAccepted == 3)
> +      finish();

There are three finish() calls in this function. We should streamline this and ensure that activeGroup's close subscriber and the tabviewhidden listener get removed when finishing the test.

@@ +106,5 @@
> +         "Only one group is open");
> +
> +      groupClosed = true;
> +      if (tabViewHidden && dialogsAccepted == 3)
> +        finish();

(see above)

@@ +121,5 @@
> +
> +      if (dialogsAccepted < 3)
> +        startCallbackTimer();
> +      else if (tabViewHidden && groupClosed)
> +        finish();

(see above)

@@ +137,5 @@
> +function setupAndRun(contentWindow, activeGroup, callback) {
> +  let closeButton = activeGroup.container.getElementsByClassName("close");
> +  ok(closeButton[0], "Group close button exists");
> +  // click the close button
> +  EventUtils.sendMouseEvent({ type: "click" }, closeButton[0], contentWindow);

Please add waitForFocus(...) before using .sendMouseEvent().

@@ +140,5 @@
> +  // click the close button
> +  EventUtils.sendMouseEvent({ type: "click" }, closeButton[0], contentWindow);
> +
> +  let onTabViewHidden = function() {
> +    window.removeEventListener("tabviewhidden", onTabViewHidden, false);

(can be removed)

@@ +147,5 @@
> +      callback(doc);
> +    };
> +    startCallbackTimer();
> +  };
> +  window.addEventListener("tabviewhidden", onTabViewHidden, false);

whenTabViewIsHidden(onTabViewHidden);

@@ +152,5 @@
> +
> +  closeUndoButton = activeGroup.$undoContainer[0].getElementsByClassName("close");
> +  ok(closeUndoButton[0], "Group undo close button exists");
> +  // click the undo close button
> +  EventUtils.sendMouseEvent({ type: "click" }, closeUndoButton[0], contentWindow);

waitForFocus(...);

@@ +158,5 @@
> +
> +// Copied from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/mochitest/prompt_common.js
> +let observer = {
> +  QueryInterface : function (iid) {
> +    const interfaces = [Ci.nsIObserver, Ci.nsISupports, Ci.nsISupportsWeakReference];

You could just use "QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupports, Ci.nsISupportsWeakReference])" here.

@@ +180,5 @@
> +   const dialogDelay = 10;
> +
> +   // Use a timer to invoke a callback to twiddle the authentication dialog
> +   let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +   timer.init(observer, dialogDelay, Ci.nsITimer.TYPE_ONE_SHOT);

The timer could be GC'ed before it fired. https://developer.mozilla.org/en/QA/Avoiding_intermittent_oranges#Using_objects_without_accounting_for_the_possibility_of_their_death

You could use .initWithCallback() here to avoid all that observer hassle.

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

Taking this one after talking to Mihai on IRC.

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

Comment 43

[Dietrich Ayala (:dietrich)]

Comment on attachment 538130 [details] [diff] [review]
patch v5

>+  _unhide: function GroupItem__unhide(options) {
>     let self = this;
> 
>     this._cancelFadeAwayUndoButtonTimer();
>     this.hidden = false;
>     this.$undoContainer.remove();
>     this.$undoContainer = null;
>     this.droppable(true);
>     this.setTrenches(this.bounds);
> 
>-    iQ(this.container).show().animate({
>-      "-moz-transform": "scale(1)",
>-      "opacity": 1
>-    }, {
>-      duration: 170,
>-      complete: function() {
>-        self._children.forEach(function(child) {
>-          iQ(child.container).show();
>-        });
>+    let finalize = function () {
>+      self._children.forEach(function(child) {
>+        iQ(child.container).show();
>+      });
> 
>-        UI.setActive(self);
>-        self._sendToSubscribers("groupShown", { groupItemId: self.id });
>-      }
>-    });
>+      UI.setActive(self);
>+      self._sendToSubscribers("groupShown", { groupItemId: self.id });
>+    };

nit: could use bind() instead of self here now. up to you.

supernit: if not using bind(), self could be defined down closer to it's actual call-site.

>+    if (!closed) {
>+      // Remove the new tab, if this group is no longer closed.
>+      tabItem.close();
>+    } else if (UI.isTabViewVisible()) {
>+      // The group is closed, we can zoom into the new empty tab.
>+      tabItem.zoomIn(true);
>+    } else {
>+      UI.goToTab(tab);
>+    }

nit: comment on the final else explaining that case.

>   // ----------
>   // Function: destroy
>   // Close all tabs linked to children (tabItems), removes all children and 
>   // close the groupItem.
>   //
>   // Parameters:
>   //   options - An object with optional settings for this call.
>   //
>   // Options:
>   //   immediately - (bool) if true, no animation will be used
>+  //
>+  // Returns true if the groupItem has been closed, or false otherwise.

Can you comment as to why destroy would *not* close, so that future devs aren't all wtf about this?

>   // Function: newTab
>   // Creates a new tab within this groupItem.
>-  newTab: function GroupItem_newTab(url) {
>+  //
>+  // Parameters:
>+  //   options - control how the new tab is added to the group.
>+  //
>+  // Possible options:
>+  //   dontZoomIn - (boolean) true if you do not want the new tab to be
>+  //                activated, zoomed into.

what *does* happen if dontZoomIn is true?

>+      this.onDOMWillOpenModalDialog = this.onDOMWillOpenModalDialog.bind(this);
>+      gWindow.addEventListener("DOMWillOpenModalDialog",
>+        this.onDOMWillOpenModalDialog, false);
>+
>+      this._cleanupFunctions.push(function() {
>+        gWindow.removeEventListener("DOMWillOpenModalDialog",
>+          self.onDOMWillOpenModalDialog, false);
>+      });

odd indent, but if that's the style here, then ok!

>+  onDOMWillOpenModalDialog: function UI_onDOMWillOpenModalDialog(event) {
>+    if (!event.isTrusted || !this.isTabViewVisible())
>+      return;
>+
>+    let tab = this.getTabForContentWindow(event.target);
>+    if (!tab)
>+      return;

in what scenario can this happen? when event is from a non-browser window?

>+
>+    // When TabView is visible, we need to call onTabSelect to make sure that
>+    // TabView is hidden and that the correct group is activated. When a modal
>+    // dialog is shown for currently selected tab the onTabSelect event handler
>+    // is not called, so we need to do it.
>+    if (gBrowser.selectedTab == tab && this._currentTab == tab)
>+      this.onTabSelect(tab);
>+  },

hum, this is kinda the crux of this whole patch... but seems wonky. the tab is selected even if you cancel the dialog right? so wouldn't the right fix be to have the browser send that event?

r- for fixing the nits, adding the comments, and figuring out the above question.

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v6

(In reply to comment #43)
> supernit: if not using bind(), self could be defined down closer to it's
> actual call-site.

Done.

> >+    if (!closed) {
> >+      // Remove the new tab, if this group is no longer closed.
> >+      tabItem.close();
> >+    } else if (UI.isTabViewVisible()) {
> >+      // The group is closed, we can zoom into the new empty tab.
> >+      tabItem.zoomIn(true);
> >+    } else {
> >+      UI.goToTab(tab);
> >+    }
> 
> nit: comment on the final else explaining that case.

Done.

> >+  //
> >+  // Returns true if the groupItem has been closed, or false otherwise.
> 
> Can you comment as to why destroy would *not* close, so that future devs
> aren't all wtf about this?

Done.

> >+  //   dontZoomIn - (boolean) true if you do not want the new tab to be
> >+  //                activated, zoomed into.
> 
> what *does* happen if dontZoomIn is true?

Corrected.

> >+  onDOMWillOpenModalDialog: function UI_onDOMWillOpenModalDialog(event) {
> >+    if (!event.isTrusted || !this.isTabViewVisible())
> >+      return;
> >+
> >+    let tab = this.getTabForContentWindow(event.target);
> >+    if (!tab)
> >+      return;
> 
> in what scenario can this happen? when event is from a non-browser window?

(event.isTrusted == true) when the event originates from a user action. It's false when generated by a script. Added comment for clarification.

> >+    // When TabView is visible, we need to call onTabSelect to make sure that
> >+    // TabView is hidden and that the correct group is activated. When a modal
> >+    // dialog is shown for currently selected tab the onTabSelect event handler
> >+    // is not called, so we need to do it.
> >+    if (gBrowser.selectedTab == tab && this._currentTab == tab)
> >+      this.onTabSelect(tab);
> >+  },
> 
> hum, this is kinda the crux of this whole patch... but seems wonky. the tab
> is selected even if you cancel the dialog right? so wouldn't the right fix
> be to have the browser send that event?

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2597

The tabbrowser activates the tab that will open a modal dialog. If the tab is already the selected one then no TabSelect event is fired and we just call this.onTabSelect() which includes the code we need (hide TabView, unhide the groupItem, set active groupItem). We could do these actions in UI.onDOMWillOpenModalDialog() but that would be some duplicated code. On the other hand it would be easier to find out which actions are taken when onDOMWillOpenModalDialog is fired.

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v7

Test corrected.

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v8

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v9

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 544282 [details] [diff] [review]
patch v9

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=dac8e8c9b06d

Comment 49

[Dietrich Ayala (:dietrich)]

Comment on attachment 544282 [details] [diff] [review]
patch v9

Review of attachment 544282 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

::: browser/base/content/tabview/groupitems.js
@@ +748,5 @@
> +  // Parameters:
> +  //   options - various options (see below)
> +  //
> +  // Possible options:
> +  //   immediately - true when no animations should be used

should say what the default is.

@@ +839,5 @@
>    // Parameters:
>    //   options - An object with optional settings for this call.
>    //
>    // Options:
>    //   immediately - (bool) if true, no animation will be used

ditto

::: browser/base/content/tabview/ui.js
@@ +943,5 @@
> +      this.onTabSelect(tab);
> +  },
> +
> +  // ----------
> +  // Function: getTabForContentWindow

this function has no Panorama-specific code at all. is there a utils object that we could move this to? or maybe browser.js?

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #49)
> ::: browser/base/content/tabview/ui.js
> @@ +943,5 @@
> > +      this.onTabSelect(tab);
> > +  },
> > +
> > +  // ----------
> > +  // Function: getTabForContentWindow
> 
> this function has no Panorama-specific code at all. is there a utils object
> that we could move this to? or maybe browser.js?

Yeah, that's true. This is an almost 1:1 copy from https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#327

Where would be a good place to merge them?

Comment 51

[Dão Gottwald [:dao]]

(In reply to comment #50)
> Where would be a good place to merge them?

Certainly not non-tabview code, since it's not e10n-proof.

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #51)
> (In reply to comment #50)
> > Where would be a good place to merge them?
> 
> Certainly not non-tabview code, since it's not e10n-proof.

I thought simply accessing .contentWindow is implemented through CPOW wrappers? How is this problem going to be solved in the <tabbrowser> itself? That has essentially the same problem:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2590

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #50)
> > > Where would be a good place to merge them?
> > 
> > Certainly not non-tabview code, since it's not e10n-proof.
> 
> I thought simply accessing .contentWindow is implemented through CPOW
> wrappers?

This is to be avoided if possible and certainly shouldn't be done in a loop querying every single browser.

> How is this problem going to be solved in the <tabbrowser> itself?
> That has essentially the same problem:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#2590

The content process needs to listen for the event, send a message to the chrome process, which can associate it with a browser (and the browser with a tab).

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > (In reply to comment #50)
> > > > Where would be a good place to merge them?
> > > 
> > > Certainly not non-tabview code, since it's not e10n-proof.
> > 
> > I thought simply accessing .contentWindow is implemented through CPOW
> > wrappers?
> 
> This is to be avoided if possible and certainly shouldn't be done in a loop
> querying every single browser.

Gotcha.

> The content process needs to listen for the event, send a message to the
> chrome process, which can associate it with a browser (and the browser with
> a tab).

Will the content process propagate the DOMWillOpenModalDialog event to the chrome process? Can we fix this by adding a DOMWillOpenModalDialog to every tab's linkedBrowser?

Comment 55

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #54)
> Will the content process propagate the DOMWillOpenModalDialog event to the
> chrome process? Can we fix this by adding a DOMWillOpenModalDialog to every
> tab's linkedBrowser?

That should read: Can we fix this by adding a DOMWillOpenModalDialog listener to every tab's linkedBrowser?

Comment 56

[Dão Gottwald [:dao]]

/Something/ could probably forward events in some fashion, but the content script can't send events, only messages.

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10

The patch is now e10s-safe. It loads a frame script that listens for DOMWillOpenModalDialog and sends a sync message to the listening browser process. The message origin (the <browser>) is passed as an argument to the listener. Thanks for pointing this out, Dão!

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 546740 [details] [diff] [review]
patch v10

>--- /dev/null
>+++ b/browser/base/content/tabview/content.js
>@@ -0,0 +1,9 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */

Our code is tri-licensed. http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10a

License header fixed.

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10b

Small correction because I broke some tests.

Comment 61

[Tim Taubert [:ttaubert] (inactive)]

Dão, Gavin, others:

Is there a specific rule about how messages should be named? Should I name it "Panorama:DOMWillOpenModalDialog" instead of "DOMWillOpenModalDialog"? I guess once the tabbrowser e10s rewrite starts it will send a similar message to the chrome process and there could be duplicates.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

I think it's best to err on the side of over-specificity, at least for the moment. Once we have more message users (whose requirements of the message details are fully fleshed out), we can investigate consolidation.

Comment 63

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10c

Message name changed to "Panorama:DOMWillOpenModalDialog".

Comment 64

[Dietrich Ayala (:dietrich)]

Comment on attachment 547531 [details] [diff] [review]
patch v10c

Review of attachment 547531 [details] [diff] [review]:
-----------------------------------------------------------------

code changes look fine, r=me. don't need to address this in the patch, but it'd be nice if we identified content scripts with a naming convention that makes them easily identifiable/searchable.

Comment 65

[Dão Gottwald [:dao]]

Comment on attachment 547531 [details] [diff] [review]
patch v10c

>     // When a tab is opened, create the TabItem
>     this._eventListeners["open"] = function(tab) {
>-      if (tab.ownerDocument.defaultView != gWindow || tab.pinned)
>+      if (tab.ownerDocument.defaultView != gWindow)
>         return;
> 
>-      self.link(tab);
>+      let frameScript = "chrome://browser/content/tabview-content.js";
>+      let messageManager = tab.linkedBrowser.messageManager;
>+      messageManager.loadFrameScript(frameScript, true);
>+
>+      if (!tab.pinned)
>+        self.link(tab);
>     }

Why aren't you loading the frame script in one central spot using the window's message manager?

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v11

(In reply to comment #64)
> it'd be nice if we identified content scripts with a naming convention that
> makes them easily identifiable/searchable.

Do you have any idea what a good naming convention could be? Maybe we should even find a place/folder to store all frame scripts in.

(In reply to comment #65)
> Why aren't you loading the frame script in one central spot using the
> window's message manager?

Didn't know that is possible, but of course that totally makes sense and is much better. I moved the frame script loading code to UI.init() where it is now done once when the Panorama frame is loaded.

Comment 67

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 68

[Dietrich Ayala (:dietrich)]

Comment on attachment 547931 [details] [diff] [review]
patch v11

Review of attachment 547931 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. i posted to DAF about a common approach for where to put the frame script files.

Comment 69

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/37d01a2d846c

Comment 70

[Tim Taubert [:ttaubert] (inactive)]

Backed out because waitForFocus() times out :(

http://hg.mozilla.org/integration/fx-team/rev/2bf2c0cc8d43

Comment 71

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/37d01a2d846c

Comment 72

[:Gavin Sharp [email: [e-mail]]]

Oops, missed that I also merged a backout!
http://hg.mozilla.org/mozilla-central/rev/2bf2c0cc8d43

Comment 73

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/1608aa3ce9d3

Comment 74

[Dão Gottwald [:dao]]

Backed out, as this appears to have significantly increased the browser.xul and about:blank leaks during mochitest-browser-chrome (tracked in bug 658738). The changesets pushed along with this look innocent to me.

Comment 75

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #74)
> Backed out, as this appears to have significantly increased the browser.xul
> and about:blank leaks during mochitest-browser-chrome (tracked in bug
> 658738). The changesets pushed along with this look innocent to me.

Thanks for noticing. Debug builds after the backout show the "normal" DOMWINDOW count again. Will investigate.

Comment 76

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/8579b0386cee

Comment 77

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/8579b0386cee