Closed Bug 1323987 Opened 7 years ago Closed 7 years ago

about:blank and about:newtab aren't restored by Session Restore

Categories

(Firefox :: Session Restore, defect)

53 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- verified
firefox54 --- verified

People

(Reporter: Virtual, Assigned: mlongaray)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss, nightly-community, regression)

Attachments

(5 files, 2 obsolete files)

STR:
1. Save Firefox browsing session with about:blank and about:newtab tabs opened
2. Restart Firefox and restore latest session
3. See that about:blank and about:newtab tabs are missing and aren't restored



[Tracking Requested - why for this release]: Regression
Flags: needinfo?(mlongaray)
Why is this dataloss, Alice? What's in about:blank or about:newtab that you need to see restored?
Flags: needinfo?(virtual)
Sorry, I meant Virtual.
I think this bug is an expected behavior after we landed bug 1306294, isn't it? Or we shouldn't be messing with our session data and we're causing a regression indeed?
Flags: needinfo?(mlongaray) → needinfo?(mdeboer)
That's why I'm asking the reporter...
Flags: needinfo?(mdeboer)
On startup there's an option called "Show my windows and tabs from last time" so the user may expect to have the exact same windows and tabs after restarting the browser, including about:newtab, about:blank or whatever they have opened.

Otherwise just change the option to "Show some of my windows and tabs from last time".
I think it's up for debate, actually, whether it's useful to have about:home and about:newtab re-opened.
NB: when the user actually typed something in the address bar in an about:home or about:newtab tab, it should restore that tab with the last user value in the address bar.

Is about:blank truly a tab? Or is it just a placeholder to show whilst you make use of it?
In which case, it'd be more apt to rename the option to 'Show my used windows and tabs from last time'. We're trying to make these kind of features more useful, however slightly, and restoring useless tabs doesn't feel like something we should keep.

In other words: this feels like a net win to me.
"Show my used windows and tabs from last time": so if you haven't used a tab or window it'll disappear on the next browser restart? that will be odd (i know it's not what you mean but it's what it looks like in that description).

As i said preserving the exact same tabs and windows and their proper order seems to be the proper way to do it and it's how it works on stable right now, i can't see any reason to change this behavior and there's no reason to mess with the windows the user have opened.
(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Why is this dataloss, Alice? What's in about:blank or about:newtab that you
> need to see restored?

Because I'm loosing data here, as session restored is not identical copy,
per missing tabs like blank tabs and new tabs, which I'm using as separators.
This could be also affected other about: addresses, especially pinned ones.

Also this is enhancing issue in bug #610357.
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(virtual)
Whiteboard: [parity-Chrome] [parity-Safari] [parity-IE] [parity-Opera] [parity-Vivaldi]
Remember that the only affected tabs would be tabs containing:

- only one entry into its navigation history
- url equals to "about:blank" or "about:newtab" or "about:privatebrowsing"
- no user typed value in the address bar

This method is defined here http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#4189, and it was already used to manage our list of closed tabs. See here http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#841
Matheus, can you just change it to still explicitly include about:blank and about:newtab pages?

(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #8)
> Because I'm loosing data here, as session restored is not identical copy,
> per missing tabs like blank tabs and new tabs, which I'm using as separators.
> This could be also affected other about: addresses, especially pinned ones.

Nope, it doesn't affect anything else. I hadn't thought about the use case of using blank tabs as separators. I'd guess nobody would have.

> Also this is enhancing issue in bug #610357.

Not true, that's a completely separate, unrelated issue.
Severity: critical → normal
Whiteboard: [parity-Chrome] [parity-Safari] [parity-IE] [parity-Opera] [parity-Vivaldi]
Flags: needinfo?(mlongaray)
So it will for sure enhance issue in bug #610357,
as we had at least information that some tab didn't load because it was blank,
but now we will loose it after session restoration because it was just blank.
Severity: normal → critical
Whiteboard: [parity-Chrome] [parity-Safari] [parity-IE] [parity-Opera] [parity-Vivaldi]
Severity: critical → normal
Whiteboard: [parity-Chrome] [parity-Safari] [parity-IE] [parity-Opera] [parity-Vivaldi]
(In reply to Mike de Boer [:mikedeboer] from comment #10)
> Matheus, can you just change it to still explicitly include about:blank and
> about:newtab pages?

I think we could, but then I'm afraid bug 1306294 would not make sense to get fixed since the browser could still end up with one extra tab (about:blank) if there is a crash or restart while print previewing (or two tabs, if simplify feature is on).

Either way, technically speaking, we could change _shouldSaveTabState somehow (while updating wherever this method is called) or create a new method.

I was wondering if backing out to WIP #1 (bug 1306294) would be another case to evaluate, i.e., only remove not worthy tabs when we detect a crash on the previous session.
Flags: needinfo?(mlongaray)
Updating flags so we don't track this in regression triage meetings.
As discussed on this bug, we are not allowed to remove "about:blank" and "about:newtab" tabs from our session state when saving it to disk - these tabs should be persisted.

Since preview opens up "about:blank" tab in background to put the document into print preview mode, we are adding a new entry in our about: redirector called "about:printpreview". In this way, when we are print previewing and session store kicks in to save our opened tabs to disk, session store will discard this kind of tab - not saving it to disk.

This patch updates our browser code to use about:printpreview and makes expected changes to SessionStore. It also updates the internal notion of nsDocShell to manually set our document's URI when creating a blank document for print preview content viewer.
Attachment #8823622 - Flags: feedback?(mdeboer)
Attachment #8823622 - Flags: feedback?(mconley)
Comment on attachment 8823622 [details] [diff] [review]
WIP1: Add about:printpreview redirector

On -175,17 +180,19 @@ AboutRedirector::NewChannel(nsIURI* aURI:

I tested URI_NORELATIVE flag (protocol doesn't support relative URIs, e.g., about and javascript) instead and it happens to work in our case. What I'm not sure is that if we are going to be causing repercussions using this flag - other than about:printpreview. See more at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolHandler.
Here's the treeherder link I pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7eebc96850661982657a008a3cd43c1dbf6cac2&group_state=expanded.

We have browser_819510_perwindowpb.js test failing. It passes locally (Linux) but it does not pass on try server. I'll be checking what's wrong.
(In reply to Matheus Longaray (:mlongaray) from comment #16)
> Here's the treeherder link I pushed to try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a7eebc96850661982657a008a3cd43c1dbf6cac2&group_state=e
> xpanded.
> 
> We have browser_819510_perwindowpb.js test failing. It passes locally
> (Linux) but it does not pass on try server. I'll be checking what's wrong.

Found the issue - myself-caused. I actually did not have to update indexes for this test. Tests should pass now. I'll post yet another treeherder link as well as use MozReview for this one.
Comment on attachment 8823622 [details] [diff] [review]
WIP1: Add about:printpreview redirector

Removing feedback flags as we're now using MozReview board.
Attachment #8823622 - Flags: feedback?(mdeboer)
Attachment #8823622 - Flags: feedback?(mconley)
Attachment #8823622 - Attachment is obsolete: true
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

https://reviewboard.mozilla.org/r/102650/#review103052

The idea looks okay to me, but I have a suggestion on how we can not special-case about:printpreview - see below.

::: browser/components/about/AboutRedirector.cpp:188
(Diff revision 1)
>        bool isUIResource = false;
>        rv = NS_URIChainHasFlags(tempURI, nsIProtocolHandler::URI_IS_UI_RESOURCE,
>                                 &isUIResource);
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> -      nsLoadFlags loadFlags = isUIResource
> +      bool isAboutPrintPreview = path.EqualsLiteral("printpreview");

Special-casing about:printpreview feels a bit icky to me. I know we special case newtab above, but that's because newtab is super _duper_ special.

I wonder if it'd be better to generalize this differently, and say: "if we're redirecting to about:blank, we should use LOAD_NORMAL".

I say this, because I can't think of a single case where we have an about module that directs to about:blank where we actually want the URL to be updated to about:blank after the redirection.

So maybe along with isUIResource, we could also have an isAboutBlank via NS_IsAboutBlank(aURI) on tempURI, and or those together instead.
Attachment #8824104 - Flags: review?(mconley) → review-
Comment on attachment 8824106 [details]
Bug 1323987 - Update browser printing code accordingly.

https://reviewboard.mozilla.org/r/102654/#review103058

Looks good!
Attachment #8824106 - Flags: review?(mconley) → review+
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review103060

::: docshell/base/nsDocShell.cpp:14235
(Diff revision 1)
> +    NS_NewURI(getter_AddRefs(uri), NS_LITERAL_CSTRING("about:printpreview"));
> +    nsresult rv = CreateAboutBlankContentViewer(principal, uri);

This is going to break for XUL apps that don't have about:printpreview set, like Thunderbird, SeaMonkey, etc.

Choices are either:

1. Make every Gecko app use about:printpreview and keep this patch as is. This will mean putting the about redirection here within docshell/base/nsAboutRedirector.cpp. I'm not totally averse to this, but this means alerting those communities so that they update their front-end code appropriately.
2. Check to see whether the document's current URI is about:printpreview, and then (and only then) use about:printpreview as the base URI on the blank content viewer that's created.

Let's see what Mossop thinks.
Attachment #8824107 - Flags: review?(mconley) → review-
Hey Mossop,

We're trying to add a new about: page for about:printpreview, so that we know not to ever persist it to SessionStore. This was the solution we ended up agreeing on with mikedeboer.

So mlongaray has done the work of adding the new redirection (essentially aliases about:printpreview to about:blank). The problem is that once docShell.printPreview.enterPrintPreview is called, the content viewer is replaced with _another_ about:blank here:

http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/docshell/base/nsDocShell.cpp#14225-14234

and the URL changes back to about:blank, and so SessionStore persists it again.

He has a patch that causes us to re-set the newly created blank content viewer's base URI to about:printpreview again, but I've expressed worries (see comment 26).

So here are the options I'm wondering about, and was wondering if you had feedback on. Should we:

1) Make it so that every Gecko app uses about:printpreview for the content viewer that print preview is occurring in?
2) Check if the original doc was about:printpreview, and if so, set the baseURI to about:printpreview again

or, just thought up:

3) Check to see if the original doc was about:printpreview (or something that is effectively a blank content viewer), make sure it's stopped, and just use that instead of creating a brand new one for no reason?

What do you think?
Flags: needinfo?(dtownsend)
(In reply to Mike Conley (:mconley) from comment #27)
> 3) Check to see if the original doc was about:printpreview (or something
> that is effectively a blank content viewer), make sure it's stopped, and
> just use that instead of creating a brand new one for no reason?

This sounds like a good option if it works. I don't know why we're only defining printpreview for Firefox and not all apps in this case though.
Flags: needinfo?(dtownsend)
Attachment #8824104 - Flags: review?(mdeboer)
Comment on attachment 8824105 [details]
Bug 1323987 - Keep saving about:blank and about:newtab to disk.

https://reviewboard.mozilla.org/r/102652/#review103542

r=me with comment addressed.

::: browser/components/sessionstore/SessionStore.jsm:4214
(Diff revision 1)
> +   */
> +  _shouldSaveTab: function ssi_shouldSaveTab(aTabState) {
> +    // If the tab has only a transient about: history entry, no other
> +    // session history, and no userTypedValue, then we don't actually want to
> +    // write this tab's data to disk.
> +    return aTabState.entries.length &&

Well, the only thing that will remain here - for backward compat reasons - is:
```js
return aTabState.entries.length &&
  aTabState.entries[0].url == "about:printpreview" ||
  aTabState.entries[0].url == "about:privatebrowsing");
```
Attachment #8824105 - Flags: review?(mdeboer) → review+
Assignee: nobody → mlongaray
Status: NEW → ASSIGNED
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

https://reviewboard.mozilla.org/r/102650/#review104340

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1323987#c28, I think we should just use about:printpreview in all XUL apps. I think that's the best way forward. Here's how I think you should proceed:

1. Move your definition of about:printpreview out of browser/components/about/AboutRedirector.cpp and put it into docshell/base/nsAboutRedirector.cpp instead. Put your `isAboutBlank = NS_IsAboutBlank(tempURI);` check there too.
2. Make sure you can reach about:printpreview after building this from Firefox.
3. See my note in your last commit for how to proceed after this.
Attachment #8824104 - Flags: review?(mconley) → review-
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review104342

Continuing from my last review:

1. Next, inside of `nsDocShell::GetPrintPreview`, after calling ::Stop(), instead of creating an about:blank viewer right away, check to see if the current URI is either about:blank or about:printpreview. If so, then skip ahead to where we QI the mContentViewer - we're done.
2. If the URI of the page is not about:blank or about:printpreview, only then should you create a new blank content viewer. And then, I guess, pass about:printpreview as the BaseURI in its construction. I don't _think_ we need the change to `CreateAboutBlankContentViewer` that you've added.

Does that work?
Attachment #8824107 - Flags: review?(mconley) → review-
Blocks: 962433
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

https://reviewboard.mozilla.org/r/102650/#review107502
Attachment #8824104 - Flags: review?(mconley) → review+
Sorry it's taking me so long to get through these. My reviews for the last two will come in tomorrow.
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review107982

I did some investigation here to see if there was a way for the currentURI of the <xul:browser> to be "about:printpreview", but the underlying URI within the DocShell to be "about:blank" (we do something similar for about:neterror pages). Unfortunately, it looks like neterror pages are pretty special (there's some internal DocShell stuff going on [where neterror is loaded](http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/docshell/base/nsDocShell.cpp#5267) ) which I don't think we have easy access to from within the DocShell AboutRedirector where we define about:printpreview. It's probably not worth it at this point to do anything special there for this, so let's keep doing our comparison here (but rename the IsContentViewerBlank method so that it's clearer that this is supposed to be used for print preview only).

Thanks mlongaray!

::: docshell/base/nsDocShell.cpp:14358
(Diff revision 3)
>    NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
>    return NS_OK;
>  }
>  
>  bool
> +nsDocShell::IsContentViewerBlank()

Let's make it clearer that this is strictly for print preview. Let's call it:

`nsDocShell::IsContentViewerBlankForPrintPreview`

::: docshell/base/nsDocShell.cpp:14365
(Diff revision 3)
> +  nsCString spec;
> +  nsresult rv = mCurrentURI->GetAsciiSpec(spec);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv) && (spec.EqualsLiteral("about:printpreview") ||
> +                                  spec.EqualsLiteral("about:blank")));
> +  if (NS_SUCCEEDED(rv) && (spec.EqualsLiteral("about:printpreview") ||
> +                           spec.EqualsLiteral("about:blank"))) {
> +    return true;
> +  }
> +
> +  return false;

`mCurrentURI->GetSpecOrDefault` is infallible, so we can compress this all down to:

```
nsCString spec = mURI->GetSpecOrDefault();
return (spec.EqualsLiteral("about:printpreview") ||
        spec.EqualsLiteral("about:blank");
```
Attachment #8824107 - Flags: review?(mconley) → review-
Comment on attachment 8826677 [details]
Bug 1323987 - Wait top level document to be loaded prior to entering print preview.

https://reviewboard.mozilla.org/r/104572/#review107988

I think there's an edgecase here - see below.

::: toolkit/content/browser-content.js:666
(Diff revision 1)
> +      if (print.doingPrintPreview) {
> -      docShell.printPreview.printPreview(printSettings, contentWindow, this);
> +        docShell.printPreview.printPreview(printSettings, contentWindow, this);
> +      } else if (content.document.readyState != "complete") {

What if `content.document.readyState == "complete"`? We should probably enter printPreview in that case too. Perhaps the first condition should be:

```
if (print.doingPrintPreview || content.document.readyState == "complete") {
  // ...
} else if (content.document.readyState != "complete") {
  addEventListener("DOMContentLoaded", () => {
    docShell.printPreview.printPreview(printSettings, contentWindow, this);
  }, { once: true});
}
```

Note that using the `once: true` setting allows us to skip the removal of the event listener. See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener for details.
Attachment #8826677 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #44)
> Comment on attachment 8826677 [details]
> Bug 1323987 - Wait top level document to be loaded prior to entering print
> preview.
> 
> https://reviewboard.mozilla.org/r/104572/#review107988
> 
> I think there's an edgecase here - see below.

Good catch, Mike!

> Note that using the `once: true` setting allows us to skip the removal of
> the event listener. See
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/
> addEventListener for details.

That's very useful! I wasn't aware of this, always learning :) Thanks!
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review108326

Looks great, thanks!
Attachment #8824107 - Flags: review?(mconley) → review+
Comment on attachment 8826677 [details]
Bug 1323987 - Wait top level document to be loaded prior to entering print preview.

https://reviewboard.mozilla.org/r/104572/#review108328

This looks good to me too!
Attachment #8826677 - Flags: review?(mconley) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e7285465d1b
Add about:printpreview redirector. r=mconley
https://hg.mozilla.org/integration/autoland/rev/06444044d4e5
Keep saving about:blank and about:newtab to disk. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/fcf065b47fdc
Update browser printing code accordingly. r=mconley
https://hg.mozilla.org/integration/autoland/rev/48837569b4e9
Avoid creation of a blank content viewer when current viewer is already blank. r=mconley
https://hg.mozilla.org/integration/autoland/rev/77bc28c97be0
Wait top level document to be loaded prior to entering print preview. r=mconley
(In reply to Wes Kocher (:KWierso) from comment #54)
> I had to back these out for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=72003606&repo=autoland
> 
> There were a couple other bc test chunks with printing-related failures.
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 02be74108ed8f60ec548563a96f0163de3961861

Thanks, Wes. Sorry you had to back out our changes. I will take a look at those ones.
Flags: needinfo?(mlongaray)
Hey mlongaray - got your messages. If getting rid of the shortcut gets rid of the leak, let's go that route. We can file follow-up work to try to sidestep the extra work in the future.

Sound okay?
Flags: needinfo?(mlongaray)
Sounds good, let's do that. Only thing we would need to add then is to re-set our document's URI to about:printpreview - since we will continue to create a brand new blank content viewer (no shortcut).

How does that sound?
Flags: needinfo?(mlongaray) → needinfo?(mconley)
Attachment #8826677 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review114634

::: docshell/base/nsDocShell.cpp:14353
(Diff revision 5)
> +    // Here we manually set current URI since we have just created a
> +    // brand new content viewer (about:blank) to host preview.
> +    SetCurrentURI(uri, nullptr, true, 0);

Right, but isn't it already set at "about:printpreview" because of the argument you passed on line 14350?
Attachment #8824107 - Flags: review+ → review?(mconley)
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review114634

> Right, but isn't it already set at "about:printpreview" because of the argument you passed on line 14350?

CreateAboutBlankContentViewer sets document URI after creating a blank document (to about:blank in this case). See here: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8123

I wonder why CreateAboutBlankContentViewer uses a base URL for relative URLs since it's a blank document.

I searched on dxr and the only place that passes a base URI to it is here: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7986

We could not even pass a base URI to CreateAboutBlankContentViewer I guess, or we could add something to CreateAboutBlankContentViewer instead. What do you suggest?

PS: when collecting a tab's data, Session Store saves the document's URL (among other stuff).
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review114634

> CreateAboutBlankContentViewer sets document URI after creating a blank document (to about:blank in this case). See here: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8123
> 
> I wonder why CreateAboutBlankContentViewer uses a base URL for relative URLs since it's a blank document.
> 
> I searched on dxr and the only place that passes a base URI to it is here: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7986
> 
> We could not even pass a base URI to CreateAboutBlankContentViewer I guess, or we could add something to CreateAboutBlankContentViewer instead. What do you suggest?
> 
> PS: when collecting a tab's data, Session Store saves the document's URL (among other stuff).

So, just so we're clear, we can get the same behaviour even without passing in the argument to CreateAboutBlankContentViewer?
ni? to mlongaray for comment 64
Flags: needinfo?(mlongaray)
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review114634

> So, just so we're clear, we can get the same behaviour even without passing in the argument to CreateAboutBlankContentViewer?

Yes. We get the same behaviour even without passing in the argument. Since we have a blank document in hand and base URLs are often used to resolve relative URLs within a document, it does not make any difference in our case to whether pass in the argument to CreateAboutBlankContentViewer.

What indeed does affect is setting the document's URI as Session Store uses it when deciding whether to keep or not a tab's data in the disk.
Flags: needinfo?(mlongaray)
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

https://reviewboard.mozilla.org/r/102656/#review115948

Okay, I'm satisfied. Let's do it!
Attachment #8824107 - Flags: review?(mconley) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6873195437cb
Add about:printpreview redirector. r=mconley
https://hg.mozilla.org/integration/autoland/rev/997080796cd3
Keep saving about:blank and about:newtab to disk. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/f372fe9889cd
Update browser printing code accordingly. r=mconley
https://hg.mozilla.org/integration/autoland/rev/43bb3f70c4d0
Set document URI after content viewer is created to host preview. r=mconley
Thank you very much! \o/
I'm marking this as VERIFIED, as it's fixed started since Mozilla Firefox 54.0a1 (2017-02-23).

Changing back importance to critical as it's dataloss bug.


@ Matheus Longaray (:mlongaray) - I'm requesting an uplift request.
Severity: normal → critical
Status: RESOLVED → VERIFIED
Flags: needinfo?(mlongaray)
(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #71)
> Thank you very much! \o/
> I'm marking this as VERIFIED, as it's fixed started since Mozilla Firefox
> 54.0a1 (2017-02-23).
> 
> Changing back importance to critical as it's dataloss bug.
> 
> 
> @ Matheus Longaray (:mlongaray) - I'm requesting an uplift request.

I am sorry but this is not fixed and data loss still occurs.
This only restores blank tabs(not their data)
Sorry for any mistakes as posting for the first time.

To reproduce the problem Use FF50 & FF54.

Also set setting: restore tabs & windows on restart
e10s on /off doesn't matter

now use this session-store file

https://1drv.ms/u/s!AqDeaDGj6aTxgb4MpSkzyOsD5grHNg

might not be totally sfw.

use FF50 and open the session.

to the right there are 75tabs & 2tabs to the left.

now click open tabs to the right, especially the tabs labeled new-tab.
They have sites stored in them(NO DATA LOSS!)

now open the same session in FF54 and click tabs and they are restored properly that is even new tabs with data/no data loss.


But now the problem can be reproduced

now open the same session in FF54 and then close FF54 and start again

see the total number of tabs they are reduced from , all tabs which sometimes shown as new-tabs even with data are removed
IMPORTANT DATA LOSS

Now this is fairly common with default profile(except restore tabs at start)
and a low speed/**** connection.
Like open multiple tabs quickly on a page and the connection of isp has low response and some pages fail to load and tabs are labelled as newtab(but stil contain data as they should of url and etc),
restarting FF50 does not loose those tabs but FF54 does loose them.

Please Fix this to work like FF50 does so none of the important data is lost due to improper connection or renaming to not properly loaded tabs etc

I am sorry if was not clear in my explanation.
if any more question are there i'll try to answer them.
just use the same session in FF50 no loss of data
in FF54 no loss first time with same session(sessionstore file is ok and working properly)


but just use the same session in FF54 & restart FF54 twice and data loss.

And FF54 does not save data which is incomplete/not totally correct
but is important still like 
improperly loaded tabs / not completely loaded tabs/ tabs stopped loading for some other reason 
and named new tab.
FF50 does this properly & so did FF53 just before this bug was filed & session store was modified

https://bugzilla.mozilla.org/show_bug.cgi?id=1323987#c0

PS- new to bugzilla so sorry for all the wrong things
@ Sunil Kumar - Thank you very much for detailed information. I can't confirm your issue on my profile with my "sessionstore.js" file. Your issue is maybe bug #610357, but looks oddly, as yours is regression... hmm. Could you try reproducing your issue on new profile, fastest way will be using Portable versions.
Flags: needinfo?(sunilbablusingh)
(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #74)
> @ Sunil Kumar - Thank you very much for detailed information. I can't
> confirm your issue on my profile with my "sessionstore.js" file. Your issue
> is maybe bug #610357, but looks oddly, as yours is regression... hmm. Could
> you try reproducing your issue on new profile, fastest way will be using
> Portable versions.

Yes i will do that tomorrow.
BTW this was a new profile but will try to freshly.
This problem started just when this bug was filed so its related i think.
https://1drv.ms/u/s!AqDeaDGj6aTxgb4NQiu_HtoNHAK0YA

Here is another one just like you asked and was able to reproduce the problem even more and better.
Just Follow like #c72

Open in FF50 : 58 tabs open and session is loaded properly.
Click on the tabs named new tab & data is loaded properly=NO DATA LOST

open in FF53/54 : restart again
from 58 tabs it goes don to 19 tabs=IMPORTANT DATA IS LOST

58 tabs to 19 tabs

IDK why but FF50 data restore is VERY RELIABLE & FF54 is just not up-to the mark.
In normal browsing , incomplete tabs and websites which do not load properly or some reason FF crashes etc then FF54 looses data & info unlike FF50 which no matter what keeps all info & restores properly

This is a major regression and users updating will unknowingly  suffer data loss!!
This should be unacceptable for FF devs as this is a major Flaw.

If it helps this problem started when this bug was filed- some other bug changed the way session restore works,
up to that point session restore was very very reliable in FF53 but then went south.

Maybe a new bug should be filed? or they bug that changed the way session restore used to work should be backed out as before that it worked fine
not sure how this works.

But this bug should not be marked as fixed .

sorry for the bit of noob input
Flags: needinfo?(sunilbablusingh)
Flags: needinfo?(mdeboer)
Flags: needinfo?(alice0775)
oops sorry for flags was removing added
Error in Browwser console on Nightly54.0a1.

Utils: Failed to deserialize principal_b64 'SmIS26zLEdO3ZQBgsLbOy9A5HoYa10qwu3wU1tmWc2k=' [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISerializationHelper.deserializeObject]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal :: line 140"  data: no]
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHEntry.triggeringPrincipal]  SessionHistory.jsm:432



Utils: Failed to deserialize principal_b64 'ZT4OTT7kRfqycpfCC8AeuAAAAAAAAAAAwAAAAAAAAEYBLyd8AA6vTdu5NkEya6SKrpIHOlRteE8wkTq4cYEyCMYAAAAABWFib3V0AAAABWJsYW5rAODaHXAvexHTjNAAYLD8FKOSBzpUbXhPMJE6uHGBMgjGAAAAAA5tb3otc2FmZS1hYm91dAAAAAVibGFuawAB3pRy0IA0EdOTmQAQS6D9QAAAAAAAAAAAwAAAAAAAAEYAAAAB//////////8BAAAAJGNocm9tZTovL2Jyb3dzZXIvY29udGVudC9icm93c2VyLnh1bAAAAAAAAAAGAAAACQAAAAcAAAAJ/////wAAAAn/////AAAACQAAAAcAAAAQAAAAFAAAABAAAAAUAAAAEAAAAAkAAAAZAAAABwAAACEAAAADAAAAAP////8AAAAQ/////wAAABD/////AQAAAAAAAAAAAAEAAAAAAAA=' [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISerializationHelper.deserializeObject]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal :: line 140"  data: no]



@sunilbablusingh@outlook.com, 
please file a new bug anyway.
Flags: needinfo?(alice0775) → needinfo?(sunilbablusingh)
(In reply to Alice0775 White from comment #78)
> Error in Browwser console on Nightly54.0a1.
> 
> Utils: Failed to deserialize principal_b64
> 'SmIS26zLEdO3ZQBgsLbOy9A5HoYa10qwu3wU1tmWc2k=' [Exception... "Component
> returned failure code: 0x80004002 (NS_NOINTERFACE)
> [nsISerializationHelper.deserializeObject]"  nsresult: "0x80004002
> (NS_NOINTERFACE)"  location: "JS frame ::
> resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal ::
> line 140"  data: no]
> NS_ERROR_FAILURE: Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsISHEntry.triggeringPrincipal]  SessionHistory.jsm:432
> 
> 
> 
> Utils: Failed to deserialize principal_b64
> 'ZT4OTT7kRfqycpfCC8AeuAAAAAAAAAAAwAAAAAAAAEYBLyd8AA6vTdu5NkEya6SKrpIHOlRteE8w
> kTq4cYEyCMYAAAAABWFib3V0AAAABWJsYW5rAODaHXAvexHTjNAAYLD8FKOSBzpUbXhPMJE6uHGBM
> gjGAAAAAA5tb3otc2FmZS1hYm91dAAAAAVibGFuawAB3pRy0IA0EdOTmQAQS6D9QAAAAAAAAAAAwA
> AAAAAAAEYAAAAB//////////
> 8BAAAAJGNocm9tZTovL2Jyb3dzZXIvY29udGVudC9icm93c2VyLnh1bAAAAAAAAAAGAAAACQAAAAc
> AAAAJ/////wAAAAn/////
> AAAACQAAAAcAAAAQAAAAFAAAABAAAAAUAAAAEAAAAAkAAAAZAAAABwAAACEAAAADAAAAAP////
> 8AAAAQ/////wAAABD/////AQAAAAAAAAAAAAEAAAAAAAA=' [Exception... "Component
> returned failure code: 0x80004005 (NS_ERROR_FAILURE)
> [nsISerializationHelper.deserializeObject]"  nsresult: "0x80004005
> (NS_ERROR_FAILURE)"  location: "JS frame ::
> resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal ::
> line 140"  data: no]
> 
> 
> 
> @sunilbablusingh@outlook.com, 
> please file a new bug anyway.

Sorry is that something i did wrong? :(

No idea about how to file the bug and needinfo stuff.
Depends on: 1342849
Depends on: 1343056
Flags: needinfo?(sunilbablusingh)
Mike, can you help shepherd an uplift request here when the time is right? Though we probably want to on that until the deps are sorted out first.
Flags: needinfo?(mconley)
Virtual, Sunil and Alice - Thanks for filing bug 1343056. Let's keep the discussion going over there.
Flags: needinfo?(mlongaray)
I think we want to uplift this, because it'll sort out the deps :)
The fact that Alice can reproduce bug 1342849 with the patch from this bug applied is a little worrisome.

mlongaray, do you have time to look into bug 1342849 to see what's happening?
Flags: needinfo?(mconley) → needinfo?(mlongaray)
Sure thing, I'm on it.
Flags: needinfo?(mlongaray)
No longer depends on: 1342849
@ Matheus Longaray (:mlongaray) - I'm requesting an uplift request.
Flags: needinfo?(mlongaray)
Let's address https://bugzilla.mozilla.org/show_bug.cgi?id=1343056#c31 prior to requesting an uplift.
Flags: needinfo?(mlongaray)
@ Matheus Longaray (:mlongaray) - I'm requesting an uplift request, as bug #1343056 was verified.
Flags: needinfo?(mlongaray)
Diff file is being attached separately as we will have conflict when merging to Beta due recent changes in this piece of code. Patch was merged in local-branch (Beta) and worked just fine.

This patch updates browser printing code to make use of about:printpreview when loading new tab for print preview. The same URI is used when the user makes use of simplify page feature while print previewing.
Attachment #8844608 - Flags: review?(mconley)
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1306294

[User impact if declined]: Data loss when restoring session

[Is this code covered by automated tests?]: SessionStore itself has a substantial automated test suite, but unfortunately, that suite did not catch the regression that these patches fix

[Has the fix been verified in Nightly?]: Yes, verified in m-i (already merged in Aurora)

[Needs manual test from QE? If yes, steps to reproduce]: Yes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1343056#c16 (although was manually tested by Virtual_ManPL and Sunil Kumar)

[List of other uplifts needed for the feature/fix]: Bug 1343056 is a _must_ (incremental fix)

[Is the change risky?]: Low risk

[Why is the change risky/not risky?]: Adds new alias for about:blank and restores old session store working behavior as much as possible (just a good cleanup)

[String changes made/needed]: None
Flags: needinfo?(mlongaray)
Comment on attachment 8824105 [details]
Bug 1323987 - Keep saving about:blank and about:newtab to disk.

See comment #90.
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

See comment #90.
Comment on attachment 8844608 [details] [diff] [review]
Bug 1323987 - Update browser printing code accordingly (rebased for 53 beta)

See comment #90.
Comment on attachment 8844608 [details] [diff] [review]
Bug 1323987 - Update browser printing code accordingly (rebased for 53 beta)

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

This is a re-based version of attachment 8824106 [details] for 53 (beta). Comment 90 in this bug lays out the case for uplifting this.
Attachment #8844608 - Flags: review?(mconley)
Attachment #8844608 - Flags: review+
Attachment #8844608 - Flags: approval-mozilla-aurora?
Comment on attachment 8844608 [details] [diff] [review]
Bug 1323987 - Update browser printing code accordingly (rebased for 53 beta)

Whoops, wrong flag.
Attachment #8844608 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8844608 - Attachment description: Bug 1323987 - Update browser printing code accordingly. → Bug 1323987 - Update browser printing code accordingly (rebased for 53 beta)
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

Details in comment 90.
Attachment #8824104 - Flags: approval-mozilla-beta?
Comment on attachment 8824105 [details]
Bug 1323987 - Keep saving about:blank and about:newtab to disk.

See comment 90.
Attachment #8824105 - Flags: approval-mozilla-beta?
Comment on attachment 8824107 [details]
Bug 1323987 - Set document URI after content viewer is created to host preview.

See comment 90.
Attachment #8824107 - Flags: approval-mozilla-beta?
Comment on attachment 8824104 [details]
Bug 1323987 - Add about:printpreview redirector.

Seems like a core scenario, recent regression, verified in 54, Beta53+
Attachment #8824104 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8824105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8824107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844608 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
See Also: → 1449956
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.