1256194 - ESR38.7.0 breaks session restore of about:preferences tab

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:  session restore is broken

Steps To Reproduce:
0. Make "Show my windows and tabs from last time" in option
1. Open option (Tools > Options)
2. Restart

Actual Results:
LocationBar indicates "jar:file:///xxxxxx/browser/omni.ja!/chrome/browser/content/browser/preferences/in-content/preferences.xul" instead "about:preferences"


Expected Results:
LocationBar should indicate "about:preferences"


Regression window:
https://hg.mozilla.org/releases/mozilla-esr38/pushloghtml?fromchange=43cd30a17135&tochange=df519be857c8

Suspect: Bug 1246956

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't know what David has to do with this...

Dragana, could you take a look, please?

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alice0775 White, thanks for flagging this one. Hi Florin, can you guys please try to repro this issue and let me know if this is something that is broken across all platforms and a consistent repro? Thanks!

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1256194_v1.patch

Boris, can you please! I will ping dteller and mark.finkle as well. Maybe it is going to be uplifted today so hope someone have time.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8730324 [details] [diff] [review]
bug_1256194_v1.patch

Change is correctly applied to mobile's SessionStore. I defer to Boris for correctness of the fix itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8730324 [details] [diff] [review]
bug_1256194_v1.patch

This needs a better commit message explaining what's really going on.

So the problem only appears when doing a restore combined with an update across versions that includes bug 1246956?  If that's _not_ what's going on here, then I don't understand what this patch is trying to do...

Comment 6

[Boris Zbarsky [:bzbarsky]]

And even if this only happens across such an update, I'm still not clear on why this fix helps.  How is loadReplace ending up true?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, wait.  It's (incorrectly) ending up false, right?  But still, how does this patch help that?

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1256194_v1.patch

Sorry this is the right solution. A variable was not initialised.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florin, I hope SoftVision can add this to their test plan so we can catch these in our sign-offs in future.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8730338 [details] [diff] [review]
bug_1256194_v1.patch

Should be false, not 0, right?

This makes a lot more sense; I assume this hunk of diff did not apply and was missed?  r=me with the false thing.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1256194_v1.patch

Comment 12

[Dragana Damjanovic [:dragana]]

Comment on attachment 8730358 [details] [diff] [review]
bug_1256194_v1.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Regression from bug 1246956(sec-high).
User impact if declined: Loading from history can show a wrong url in the location bar. 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): low only a variable  is initialized, that was missed in the last patch
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8730338 [details] [diff] [review]
> bug_1256194_v1.patch
> 
> Should be false, not 0, right?
> 
> This makes a lot more sense; I assume this hunk of diff did not apply and
> was missed?  r=me with the false thing.

Yes, I miss that somehow. All this file where moved from one to other folder so patch could not be applied automatically... and somehow I miss that line. 
47, 46,45  does not have this bug.

Comment 14

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Ritu Kothari (:ritu) from comment #9)
> Hi Florin, I hope SoftVision can add this to their test plan so we can catch
> these in our sign-offs in future.

We can, yes. Moving to Andrei just for the sake of tracking, as he already knows about this.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730358 [details] [diff] [review]
bug_1256194_v1.patch

This will be a ride-along (severe enough) to be included in esr38.7.1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/543165802342

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #14)
> (In reply to Ritu Kothari (:ritu) from comment #9)
> > Hi Florin, I hope SoftVision can add this to their test plan so we can catch
> > these in our sign-offs in future.
> 
> We can, yes. Moving to Andrei just for the sake of tracking, as he already
> knows about this.

We've added a test case for this specific scenario in our Session Restore Test Suite. We'll make sure it gets extra attention through Exploratory Testing, starting with the validation of 38.7.1esr.

Comment 18

[Vlad Bacia-Mociran [:VladB]]

I have tested the issue on Ubuntu 14.04 x64, Mac OS X 10.11, Windows 8.1 x64 with Fx 38.7.1esr candidate (Build ID: 20160315145633)and could not reproduce it. 

I have followed the steps from description and after browser restart, "about:preferences" is displayed in the LocationBar. I've also tried to replicate this issue with other internal pages and everything looks as expected.