841151 - Tab from last time saved as wyciwyg://

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: What you see is what you get (screenshot)

Not sure what happened here; restored Nightly and my single open tab was saved with the prefix 'wyciwyg://'. Accessing this tab  fails:

E/GeckoConsole(14281): Handled load error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: loadURIWithFlags :: line 156"  data: no]

Comment 1

[Aaron Train [:aaronmt]]

Ok, that was easy.

STR:

i) http://wired.com
ii) Swipe to close Fennec
iii) Re-open Fennec

See tabs from last time on about:home

Comment 2

[:Margaret Leibovic]

We're reading from session storage to get this URL:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutHomeContent.java#631

It looks like we try to avoid storing these URLs in session storage:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#666

But I added some logging, and didn't see the wyciwyg:// URL go through that code path. So something else is writing that into the session storage we're reading in AboutHomeContent.

Comment 3

[:Margaret Leibovic]

Also see bug 795317. We may want to do something with URIFixup to fix this.

Comment 4

[:Margaret Leibovic]

Investigating this a bit more, I found that we're actually saving a top-level wyciwyg entry in _serializeHistoryEntry, so the check down below isn't at fault.

I tried reproducing on desktop with a fennec UA, but I didn't see the wyciwyg entry in my sessionstore.js. Comparing fennec's SessionStore.js to desktop's SessionRestore.jsm, it's not clear why this isn't a desktop issue as well, since they're both just iterating through browser.sessionHistory.

A simple fix would be to avoid serializing wyciwyg entries, but I think it would be better to figure out why this isn't happening on desktop.

(As an aside, I found some improvements to the desktop SessionStore.jsm that didn't make their way into fennec's SessionStore.js. Maybe this would be a good time to look for fixes to take into fennec.)

Comment 5

[:Margaret Leibovic]

Yesterday I added some logging to a desktop build, and I was actually able to verify that the same thing was happening as on fennec. Maybe something was wrong with my testing earlier, but I also found that session restore failed on desktop for the same reason (probably nobody ever noticed because this is a rare testcase, but we could file a desktop bug about that).

So I think we should just avoid storing top-level wyciwyg history entries, in addition to not storing them for frames (what we currently do in _serializeHistoryEntry).

Comment 6

[:Margaret Leibovic]

Attachment: patch

This patch makes session restore just ignore wyciwyg history entries. I adjusted the index to account for this, but I wasn't sure of a good way to test whether or not something may still be broken by this.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 719134 [details] [diff] [review]
patch

It seems strange that wyciwyg URIs are in the history in the first place. What's different about wired.com that's causing this? Do you think we could prevent them from being added to history altogether rather than having to prune them out later?

Comment 8

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Comment on attachment 719134 [details] [diff] [review]
> patch
> 
> It seems strange that wyciwyg URIs are in the history in the first place.
> What's different about wired.com that's causing this? Do you think we could
> prevent them from being added to history altogether rather than having to
> prune them out later?

I modeled this fix after bug 450595, which decided to just ignore these urls in session restore.

This history entry comes from browser.sessionHisotry, which comes from docShell, and I'm not sure what the implications of changing that would be. I'm wary of changing what we're storing, since it may actually be necessary for something (otherwise, why would we be making these URLs in the first place?). However, we can talk with someone who knows more about docShell to understand more about what's going on here.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 719134 [details] [diff] [review]
patch

Since desktop does this and there's an explanation in https://bugzilla.mozilla.org/show_bug.cgi?id=450595#c10, I agree that this should be good enough for us too.

But I think there's an issue with calculating the index using the skip count. If any entries after the selected entry are skipped, the selected index will be erroneously decremented. Say, for instance, the selected index is 1; if two entries after that are skipped, the adjusted index becomes -1.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Comment on attachment 719134 [details] [diff] [review]
> patch
> 
> Since desktop does this and there's an explanation in
> https://bugzilla.mozilla.org/show_bug.cgi?id=450595#c10

Err...re-reading the comments, I guess desktop doesn't do this since this is actually an extension of the existing workaround, but I'm still fine with the fix.

Comment 11

[:Margaret Leibovic]

Attachment: patch v2

Good catch about the index. This version only adjusts the index if its higher than an entry we skipped. (I made it a <=, so if the index is at an entry we're skipping, we'll just fall back to the entry before it.)

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 726335 [details] [diff] [review]
patch v2

Maybe it would be simpler to move the "let index = history.index + 1" before the loop, then do "index--" instead of "skipped++" inside the loop?

Comment 13

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #12)
> Comment on attachment 726335 [details] [diff] [review]
> patch v2
> 
> Maybe it would be simpler to move the "let index = history.index + 1" before
> the loop, then do "index--" instead of "skipped++" inside the loop?

That sounds good.

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/260c1b2a4b33

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/260c1b2a4b33

Comment 16

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 22.0a1 (2013-03-27)
-device: LG Nexus 4
-OS: Android 4.2.2