Alice0775 White Reporter
	Description • 10 years ago

Steps To Reproduce:
1. Open http://forums.mozillazine.org/viewtopic.php?f=23&t=629560 in New Tab [A]
2. Close Other Tabs
3. Create a New Tab
4. Close Tab[A]
5. Undo Close Tab

Actual Results:
Content of the tab is blank.
F5, Ctrl+F5 does not help.

	Tim Taubert [:ttaubert] (inactive)
	Comment 1 • 10 years ago

Last good nightly: 2014-08-06
First bad nightly: 2014-08-07

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6cbdd4d523a7&tochange=afcb3af79d09

	Alice0775 White Reporter
	Comment 2 • 10 years ago

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/4bd09430f063
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140805142615
Bad:
https://hg.mozilla.org/integration/fx-team/rev/06b6aa5c734d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140805144123
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4bd09430f063&tochange=06b6aa5c734d

Regressed by: Bug 1009628

	David Parks [:handyman] Assignee
	Comment 3 • 10 years ago

Ah, there is nothing like a repro.

This is a slightly more formal set of steps.  It is a 100% repro on my Macbook Pro:

* Open an e10s window
* Browse to any site
* Open a new tab (Command-t)
* Close the old tab (press the X)
* In the History menu, select the tab you just closed.

Expected result:
The new tab you opened is closed and the old tab is reopened to the correctly rendered page.

Actual result:
The new tab you opened is closed and the old tab is reopened to a white screen.

-----------

This patch fixes the issue, although I'm not sure that its the proper behavior.

	David Parks [:handyman] Assignee
	Comment 4 • 10 years ago

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=5ac56a390222

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 10 years ago

Comment on attachment 8480358 [details] [diff] [review]
Set selectedIndex attribute on tabbrowser before 'select' event on tab-switch

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

Let me poke at this for a few minutes and see if I can come up with an alternative.

::: browser/base/content/tabbrowser.xml
@@ +5338,5 @@
>            }
>  
>            this._selectedIndex = val;
>  
>            switchPromise.then(() => {

Unfortunately, moving this out of the Promise resolution functions re-introduces the problem of us synchronously switching the tab deck when changing tabs, which opens us up to seeing blank tabs when switching.

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 10 years ago

Ok, here's what's happening:

This bug is only reproducible if when restoring that tab, the only tab in the browser is about:blank or about:newtab (or whatever your newtab is set to be) - this is what browser.js considers a "blank tab".

There's special code that handles that case, because the expected behaviour is that the restored tab will take the place of that about:blank or newtab tab. In actuality, what occurs is that the old tab is restored, and we immediately close the about:blank or newtab tab - so that it looks like the restored tab replaced the lone tab.

Here's the code in browser.js that handles undoing a tab close:

/**
 * Re-open a closed tab.
 * @param aIndex
 *        The index of the tab (via SessionStore.getClosedTabData)
 * @returns a reference to the reopened tab.
 */
function undoCloseTab(aIndex) {
  // wallpaper patch to prevent an unnecessary blank tab (bug 343895)
  var blankTabToRemove = null;
  if (gBrowser.tabs.length == 1 && isTabEmpty(gBrowser.selectedTab))
    blankTabToRemove = gBrowser.selectedTab;

  var tab = null;
  if (SessionStore.getClosedTabCount(window) > (aIndex || 0)) {
    TabView.prepareUndoCloseTab(blankTabToRemove);
    tab = SessionStore.undoCloseTab(window, aIndex || 0);
    TabView.afterUndoCloseTab();

    if (blankTabToRemove)
      gBrowser.removeTab(blankTabToRemove);
  }

  return tab;
}

The blankTabToRemove is being set, and then we remove it towards the end of the function.

So that's the set up of the bug.

Here's what's going wrong:

We have the about:blank or newtab tab set in the tabpanels deck at index 0 to start. We restore the closed tab, and this gets appended to the tabpanels - the panel is inserted at index 1. SessionStore then commands the tabpanels to switch to that restored tabpanel at index 1.

Here's where the async code I added kicks in. That function to set the tabpanel to index 1 returns, but the deck hasn't yet switched its selectedIndex to 1 just yet - it's waiting for the restored tab's MozAfterRemotePaint event to do that.

While that's still in flight, we remove the original about:blank / newtab tab. The tabpanels selected index gets decremented automatically to 0.

There is now only a single tabpanel in the tabpanels deck, and it's at index 0, which is what we were already looking at to start with.

BAM, MozAfterRemotePaint fires, and tabpanels goes "Oh, we're done - I guess I'd better finish off setting our selectedIndex to 1".

Which it does - and that means the deck is set to point at an index that has no child. So nothing is displayed.

And that's the bug.

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 10 years ago

I really, really, really hope we don't have too many of these types of bugs - but it's the one I was kind of afraid of when we did that work in bug 1009628 - namely, quick manipulation of tabs that causes in-flight stuff to get out of sync.

The solution might be to have the selectedIndex setter in tabbrowser-tabpanels set a variable to the panel that we've been asked to show at index val, and then in the Promise solution, don't just blindly re-use the same index that selectedIndex was passed. Instead, re-query the childNodes for the index of the panel that we were asked to show, and set the selectedIndex to that instead.

handyman - did you want to try knocking that one out? If not, I can take it.

	David Parks [:handyman] Assignee
	Comment 9 • 10 years ago

Hey Mike, is this what you had in mind?  It does indeed fix the bug.

	Mike Conley (:mconley) (:⚙️)
	Comment 10 • 10 years ago

Comment on attachment 8481692 [details] [diff] [review]
Update tab index after select event

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

Looks good - just a few suggestions.

::: browser/base/content/tabbrowser.xml
@@ +5337,5 @@
>  
>            this._selectedIndex = val;
>  
>            switchPromise.then(() => {
> +            var updatedTabIndex = -1;

I think you can do:

var updatedTabIndex = Array.indexOf(this.childNodes, newPanel);

instead, and cut out the for-loop.

@@ +5344,5 @@
> +                updatedTabIndex = i;
> +                break;
> +              }
> +            }
> +            NS_ASSERT(updatedTabIndex != -1, "The new tab has already been removed.");

Wow - I didn't even know NS_ASSERT was available at the JS level here. Good find. :)

	David Parks [:handyman] Assignee
	Comment 11 • 10 years ago

Thanks - this patch removes the loop.  I'm setting you to review or to retarget the request.

	Mike Conley (:mconley) (:⚙️)
	Comment 12 • 10 years ago

Comment on attachment 8481734 [details] [diff] [review]
Update tab index after select event

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

::: browser/base/content/tabbrowser.xml
@@ +5338,5 @@
>            this._selectedIndex = val;
>  
>            switchPromise.then(() => {
> +            var updatedTabIndex = Array.indexOf(this.childNodes, newPanel);
> +            NS_ASSERT(updatedTabIndex != -1, "The new tab has already been removed.");

In the event that updatedTabIndex is -1, then we definitely don't want to finalize the tab switch - we'll want to cancel the tab switch, like we do in the promise rejection handler.

Would you mind putting in the code to do that in the unlikely event that updatedTabIndex is -1? (I say unlikely, but closing a tab before we've completed switching to it is something that our test suites probably love doing). If you handle that case, then this should be good to go.

	David Parks [:handyman] Assignee
	Comment 13 • 10 years ago

That's what I had initially -- I had replaced it with the NS_ASSERT.  Now, it does both.

Good point about the tests... in fact, I should probably run some.  They are coming in here:
https://tbpl.mozilla.org/?tree=Try&rev=330374c51a84

	Mike Conley (:mconley) (:⚙️)
	Comment 14 • 10 years ago

Comment on attachment 8481743 [details] [diff] [review]
Update tab index after select event

LGTM. Thanks handyman!

	Dão Gottwald [:dao]
	Comment 15 • 10 years ago

Comment on attachment 8481743 [details] [diff] [review]
Update tab index after select event

>+            var updatedTabIndex = Array.indexOf(this.childNodes, newPanel);
>+            NS_ASSERT(updatedTabIndex != -1, "The new tab has already been removed.");
>+            if (updatedTabIndex == -1) {
>+              gBrowser._cancelTabSwitch(toTab);

NS_ASSERT(updatedTabIndex != -1, ...) doesn't make sense here, since you seem to both expect and handle the case of updatedTabIndex being -1.

>+            }
>+            else {

nit: } else {

	Mike Conley (:mconley) (:⚙️)
	Comment 16 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 8481743 [details] [diff] [review]
> Update tab index after select event
> 
> >+            var updatedTabIndex = Array.indexOf(this.childNodes, newPanel);
> >+            NS_ASSERT(updatedTabIndex != -1, "The new tab has already been removed.");
> >+            if (updatedTabIndex == -1) {
> >+              gBrowser._cancelTabSwitch(toTab);
> 
> NS_ASSERT(updatedTabIndex != -1, ...) doesn't make sense here, since you
> seem to both expect and handle the case of updatedTabIndex being -1.

Hm, I agree with Dao here. Sorry for the bad feedback, handyman - this is just a case of us needing to take care of the corner case of the tab index order shifting before the MozAfterRemotePaint fires, which is rare but possible - so the assert makes less sense.

Maybe some documentation above

if (updatedTabIndex == -1)

would be better, like:

// If we cannot find the tabpanel that we were trying to switch to, then
// it must have been removed before our Promise could be resolved. In
// that case, we just cancel the tab switch.

I'd be happy with that.

	David Parks [:handyman] Assignee
	Comment 17 • 10 years ago

...with the assert removed.

	David Parks [:handyman] Assignee
	Comment 18 • 10 years ago

...last version missed Dao's style nit.

	Mike Conley (:mconley) (:⚙️)
	Comment 19 • 10 years ago

Setting checkin-needed. A try push is unlikely to reveal anything, seeing as how this code is only exercised if we're using an e10s window, and that doesn't happen right now on try.

	Carsten Book [:Tomcat]
	Comment 20 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/518ad95704fc

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/518ad95704fc

	Mike Conley (:mconley) (:⚙️)
	Comment 23 • 10 years ago
bugnotes

Bugnotes:

http://people.mozilla.org/~mconley2/bugnotes/bug-1059036.html

	Cornel Ionce [:noni] [Hubs QA]
	Comment 24 • 10 years ago

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0

Confirming this fix on latest Nightly, build ID: 20141221030204.