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

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jbecerra, Assigned: mbrubeck)

Tracking

29 Branch
Firefox 29
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28] [defect] p=3 )

Attachments

(2 attachments)

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.
Whiteboard: [triage] [defect] p=0
I noticed that it doesn't matter which tab I am on, if the master password prompt comes up the tab will not load.
Summary: defect - switching between desktop and touch mode loses tabs if master password is needed to load them → defect - switching between desktop and touch mode loses tabs if master password comes up
Whiteboard: [triage] [defect] p=0 → [beta28] [defect] p=0
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.
Depends on: 957501
Assignee: nobody → mbrubeck
Hey Matt, are you taking this bug during Iteration #22?  If so, can you provide a point value.  Thanks.
Flags: needinfo?(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.
Blocks: metrov1it22
No longer blocks: metrov1backlog
Flags: needinfo?(mbrubeck)
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=3
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Posted file sessionstore.bak
Here's a sessionstore.js file that reproduces the bug, generated by following a reduced version of the steps from comment 0.
Posted patch session-errorSplinter Review
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.)
Attachment #8360099 - Flags: review?(tabraldes)
Summary: defect - switching between desktop and touch mode loses tabs if master password comes up → Errors restoring session in Metro mode if it contains navigation within a document
Blocks: 957501
No longer depends on: 957501
Duplicate of this bug: 957011
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.
Attachment #8360099 - Flags: review?(tabraldes) → review+
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.
Whiteboard: [beta28] [defect] p=3 → [beta28] [defect] p=3 [fixed-in-fx-team]
If anyone is curious, this bug was introduced in Fennec 10 by bug 687710.
Blocks: 687710
https://hg.mozilla.org/mozilla-central/rev/59251ffa1853
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] [defect] p=3 [fixed-in-fx-team] → [beta28] [defect] p=3
Target Milestone: --- → Firefox 29
Duplicate of this bug: 958174
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.
Attachment #8360099 - Flags: approval-mozilla-aurora?
Attachment #8360099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Juan, can you please verify this is fixed in Aurora and Nightly?
Flags: needinfo?(jbecerra)
Keywords: verifyme
Blocks: 951402
Duplicate of this bug: 951402
Blocks: 956067
Duplicate of this bug: 956067
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbecerra)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.