957150 - Errors restoring session in Metro mode if it contains navigation within a document

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[juan becerra [:juanb]]

Tested on 2014-01-07 using latest nightly built from http://hg.mozilla.org/mozilla-central/rev/e7a366c1036c

I noticed that my Zimbra mail tab loaded as a blank tab when going from desktop mode to Windows Touch mode, and when I try to relaunch in desktop it disappears (because it is blank).

I have a master password set for this profile, and I've opted to save my Mozilla/Zimbra mail username and password.

Steps:
You need to have a mozilla.com email account, a master password for your profile, and you need to have opted to save your username/password for your Mozilla mail.

1. Go to https://mail.mozilla.com/
You should be prompted to enter your master password to fill in the username and password.
2. Login with your filled-in credentials.
3. Open another tab like www.nytimes.com
4. Switch back to the https://mail.mozilla.com/
5. Go to the options button and switch to touch mode.

Expected: Firefox opens in touch mode with my Zimbra tab open and it is properly loaded. If a master password is required, it should prompt me to enter the information, and if I enter it it should load my tab.

Actual: It loads a blank tab. The nytimes.com tab loads fine. Once I got prompted for a master password, which I entered, but then no tab was loaded.

I don't know how to categorize the problem, but I think that if I have just opened a mail session and if switch from desktop to touch mode, it should just work without having to re-initiate a session.

This might be related to bug 949333.

Comment 1

[juan becerra [:juanb]]

I noticed that it doesn't matter which tab I am on, if the master password prompt comes up the tab will not load.

Comment 2

[Matt Brubeck (:mbrubeck)]

Metro Firefox logs the following error on startup when I reproduce this bug:
NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISHEntry.adoptBFCacheEntry]

So it looks like this is another way to reproduce bug 957501.

Comment 3

[Marco Mucci [:MarcoM]]

Hey Matt, are you taking this bug during Iteration #22?  If so, can you provide a point value.  Thanks.

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Marco Mucci [:MarcoM] from comment #3)
> Hey Matt, are you taking this bug during Iteration #22?  If so, can you
> provide a point value.  Thanks.

Yes - p=3.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: sessionstore.bak

Here's a sessionstore.js file that reproduces the bug, generated by following a reduced version of the steps from comment 0.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: session-error

I found two bugs in our our WebNavigation._deserializeHistoryEntry.

First, the change to UUIDs in bug 927038 does not work, assigning to shEntry.ID coerces the UUID to a number, which always returns NaN.  Oops!  This patch reverts the patch from bug 927038.

Second, we were calling shEntry.adoptBFCacheEntry(matchingEntry) when we should have been passing matchingEntry.shEntry.

Both these lines now exactly match the implementations in desktop Firefox:
http://hg.mozilla.org/mozilla-central/file/12d3ba62a599/browser/components/sessionstore/src/SessionHistory.jsm#l353
http://hg.mozilla.org/mozilla-central/file/12d3ba62a599/browser/components/sessionstore/src/SessionHistory.jsm#l398

QA note: This bug wasn't actually caused by the master password dialog; it happens on any session that includes a navigation within the same page using a "#fragment" identifier in the URI.  (Zimbra does this when you log in, which is why this is easy to reproduce with mail.mozilla.com.)

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8360099 [details] [diff] [review]
session-error

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

I haven't tested locally but the code looks fine. I can apply the patch and test tomorrow if you'd like.

::: browser/metro/base/content/bindings/browser.js
@@ +264,3 @@
>          aIdMap[aEntry.ID] = id;
>          aIdMap.used[id] = true;
>        }

Is there a reason to use Date.now() and a loop?

I feel like we could accomplish equivalent behavior with this:
  if (!id) {
    id = (aIdMap.lastID || 0) + 1;
    aIdMap[aEntry.ID] = id;
    aIdMap.lastID = id;
  }
With the added benefit that readers of the code (like me) won't go searching for how/where the "used" member of aIdMap is used outside of this function.

Of course, it's probably better to mimic desktopFx than to diverge from it, and there's always the possibility that this change would cause unforeseen issues, so I'm not opposed to leaving this as is.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/59251ffa1853

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #8)
> Is there a reason to use Date.now() and a loop?

I don't *know*, but it's *possible* that there's an undocumented requirement that these not conflict with some other sequence of IDs which starts from 0, and which is generated in code that doesn't know about this code (or vice-versa).  I hope that's not the case, but I didn't want to risk it in this Aurora-bound patch.

Comment 10

[Matt Brubeck (:mbrubeck)]

If anyone is curious, this bug was introduced in Fennec 10 by bug 687710.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59251ffa1853

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8360099 [details] [diff] [review]
session-error

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Metro Firefox

User impact if declined: Session restore fails during Metro startup after using certain web pages.

Testing completed (on m-c, etc.): Landed on m-c on 01-15.

Risk to taking this patch (and alternatives if risky): Low-risk, Metro-only, mostly just reverts a previous change.

String or IDL/UUID changes made by this patch: None.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df0434426efe

Comment 15

[u279076]

Juan, can you please verify this is fixed in Aurora and Nightly?

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on Firefox 28 beta 4 (build ID:20140218122424) and on latest Aurora (build ID: 20140218004001).
Tested using the STR from the description with gmail and yahoo mail instead of mozilla mail.
Verified on my machine and on a Surface Pro 2 device, both running Windows 8.1 64bit.