Last Comment Bug 644998 - Session should not be restorable after "Clear Recent History"
: Session should not be restorable after "Clear Recent History"
Status: RESOLVED FIXED
[good first bug][mentor=zpao]
: privacy
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Firefox 8
Assigned To: Alastair Robertson
:
Mentors:
http://www.google.com/
: 650354 (view as bug list)
Depends on:
Blocks: 675493
  Show dependency treegraph
 
Reported: 2011-03-25 07:05 PDT by blacklion8888
Modified: 2011-08-22 11:52 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 - clear this._lastSessionState (1.17 KB, patch)
2011-05-31 18:37 PDT, Alastair Robertson
paul: review+
faaborg: ui‑review+
Details | Diff | Review
Patch 2 - hide session restore button on about:home (2.15 KB, patch)
2011-06-13 12:04 PDT, Alastair Robertson
paul: feedback+
Details | Diff | Review
Patch 3 - responded to feedback (2.17 KB, patch)
2011-06-20 20:40 PDT, Alastair Robertson
gavin.sharp: review+
Details | Diff | Review
Patch 4 - changed to use forEach() (2.18 KB, patch)
2011-07-14 15:06 PDT, Alastair Robertson
no flags Details | Diff | Review
Patch 5 - changed to use message manager (2.79 KB, patch)
2011-07-21 12:54 PDT, Alastair Robertson
gavin.sharp: review+
Details | Diff | Review
Patch 6 (2.82 KB, patch)
2011-07-28 13:02 PDT, Alastair Robertson
no flags Details | Diff | Review

Description blacklion8888 2011-03-25 07:05:52 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0

Do a cleaning of -all- temp data of all periods (from an empty tab). Then, you'll see that a restore of a prev. session is available.

Reproducible: Sometimes

Steps to Reproduce:
1. Open a new empty tab.
2. Clean -all- temp data.
3. Sometimes, a prev. session restore is available.
Actual Results:  
Disfunction / error.

Expected Results:  
All blank.

none
Comment 1 Josh Matthews [:jdm] 2011-03-25 08:13:51 PDT
Doesn't sound like private browsing.
Comment 2 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-25 09:26:24 PDT
Assuming you mean that a session can still be restored after clearing everything through Tools > Clear Recent History, this is as designed. I'm moving this over to enhancements so it can be considered by the Session Store developers.
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-23 11:06:47 PDT
Yea we should probably clear that.
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-23 11:11:45 PDT
*** Bug 650354 has been marked as a duplicate of this bug. ***
Comment 5 Alastair Robertson 2011-05-31 18:37:30 PDT
Created attachment 536500 [details] [diff] [review]
Patch 1 - clear this._lastSessionState


Does this bug need some automated tests to be created for it?
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-03 11:19:22 PDT
Comment on attachment 536500 [details] [diff] [review]
Patch 1 - clear this._lastSessionState

Thanks Alastair!
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-03 11:25:10 PDT
Comment on attachment 536500 [details] [diff] [review]
Patch 1 - clear this._lastSessionState

Alex - this makes it so that on about:home (assuming session just got cleared), the button can still be clicked but then it fails silently. The button goes away and nothing happens. That's probably OK for most of the time, but would it make sense to replace the button with a message saying that it couldn't be done? Removing the button is more difficult (presumably possible, but not sure how just yet). New instances of about:home will not have the button present.
Comment 8 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-03 15:10:52 PDT
Comment on attachment 536500 [details] [diff] [review]
Patch 1 - clear this._lastSessionState

I guess providing a message in place is ok as an intermediary solution, would we do it where the button was?
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-04 13:33:56 PDT
(In reply to comment #8)
> Comment on attachment 536500 [details] [diff] [review] [review]
> Patch 1 - clear this._lastSessionState
> 
> I guess providing a message in place is ok as an intermediary solution,
> would we do it where the button was?

The patch here doesn't do that but that's what I was imagining. Would you prefer we look into making the button just disappear and fallback to the message if that's not possible?

(In reply to comment #5)
> Does this bug need some automated tests to be created for it?

This is going to be difficult to test automatically because it requires a previous session, which we don't really have the ability to test easily. We'll flag it for in-litmus when it's ready to go.

Alastair, depending on what Alex says, you're going to get to explore outside of session restore. Ping me during the week and we can chat about the right places to start looking to fix this.
Comment 10 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-04 15:07:57 PDT
>The patch here doesn't do that but that's what I was imagining. Would you prefer >we look into making the button just disappear and fallback to the message if >that's not possible?

Yes, clicking on the button and then having Firefox say "actually just kidding" obviously isn't great :)  It would be better if we didn't set them up for that (and didn't have a button).
Comment 11 Alastair Robertson 2011-06-13 12:04:49 PDT
Created attachment 538976 [details] [diff] [review]
Patch 2 - hide session restore button on about:home

There seem to be a few ways of getting what page is open in a tab - contentDocument.documentURI, contentDocument.baseURI and contentDocument.URL.

I used contentDocument.documentURI, but does it make a difference which one is used?
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-14 14:52:12 PDT
Comment on attachment 538976 [details] [diff] [review]
Patch 2 - hide session restore button on about:home

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

Right idea, but it needs some cleanup. I can't actually review the changes to browser.js, but I'll make sure the people who can look at it when the new patch is up

::: browser/base/content/browser.js
@@ +355,5 @@
>      var fwdCommand = document.getElementById("Browser:Forward");
>      fwdCommand.setAttribute("disabled", "true");
>  
> +    // Hide session restore button on about:home
> +    for (let tab in gBrowser.browsers) {

Let's do this as a normal for ... i++ type of loop for speed. for...in is slow

You can loop over gBrowser.browsers or you can do gBrowser.tabs

@@ +356,5 @@
>      fwdCommand.setAttribute("disabled", "true");
>  
> +    // Hide session restore button on about:home
> +    for (let tab in gBrowser.browsers) {
> +      if (gBrowser.browsers[tab].contentDocument.documentURI === "about:home")

If you have a tab, you can do .linkedBrowser to get the browser object. Or if you're looping over .browser, you already have one.

To get the url, browser.currentURI.spec is the preferred way.
Comment 13 Alastair Robertson 2011-06-20 20:40:15 PDT
Created attachment 540671 [details] [diff] [review]
Patch 3 - responded to feedback
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-27 15:52:15 PDT
Comment on attachment 540671 [details] [diff] [review]
Patch 3 - responded to feedback

Gavin, can you take a look at the browser bits here? It looks ok to me. We may want to abstract it away in the future though so that we can use it when the button on about:home is clicked (and there are multiple about:home pages open - I think there's a bug somewhere)
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-06 13:34:10 PDT
Comment on attachment 540671 [details] [diff] [review]
Patch 3 - responded to feedback

It's somewhat unpleasant to have to iterate over all tabs and remove buttons, but I suppose there aren't any great alternatives.

I think you should use something like this instead:
gBrowser.browsers.forEach(function (b) {
  let doc = b.contentDocument, container;
  if (doc.documentURI.toLowerCase() == "about:home" &&
      (container = doc.getElementById("sessionRestoreContainer"))
    container.hidden = true;
});

r=me with that change.
Comment 16 Alastair Robertson 2011-07-14 15:06:33 PDT
Created attachment 546011 [details] [diff] [review]
Patch 4 - changed to use forEach()
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-15 10:00:16 PDT
Felipe actually pointed out quite reasonably that this code isn't very e10s friendly. Alastair, are you up to modifying it slightly so that it is? That basically involves putting the code inside that loop in a content message listener (in browser/base/content/content.js), and then sending that message to the global messageManager where that previous patch adds the loop (using window.messageManager.sendAsyncMessage). Happy to offer more advice/guidance on IRC if needed (I'm "gavin" on #fx-team).
Comment 18 Alastair Robertson 2011-07-16 15:20:21 PDT
Yes, I'll definately have a go at doing this, although I probably will need to contact you on IRC at some point.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 10:39:40 PDT
Comment on attachment 546011 [details] [diff] [review]
Patch 4 - changed to use forEach()

Excellent! I'll cancel this review request in the mean time.
Comment 20 Alastair Robertson 2011-07-21 12:54:09 PDT
Created attachment 547479 [details] [diff] [review]
Patch 5 - changed to use message manager
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-26 09:16:45 PDT
Comment on attachment 547479 [details] [diff] [review]
Patch 5 - changed to use message manager

>diff --git a/browser/base/content/content.js b/browser/base/content/content.js

>+addMessageListener("Browser:HideSessionRestoreButton", function (message) {
>+  // Hide session restore button on about:home
>+  let doc = content.document;
>+  if (doc.documentURI.toLowerCase() == "about:home" &&
>+      (container = doc.getElementById("sessionRestoreContainer"))){

You need to declare "container" here, but otherwise this looks excellent! Great work.
Comment 22 Alastair Robertson 2011-07-28 13:02:01 PDT
Created attachment 549203 [details] [diff] [review]
Patch 6
Comment 23 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-28 13:18:10 PDT
Comment on attachment 549203 [details] [diff] [review]
Patch 6

No need to ask for review again so I'm clearing that for you. Just one comment to save Gavin the time. 

>+addMessageListener("Browser:HideSessionRestoreButton", function (message) {
>+  // Hide session restore button on about:home
>+  let doc = content.document;
>+  if (doc.documentURI.toLowerCase() == "about:home" &&
>+      doc.getElementById("sessionRestoreContainer")) {
>+    doc.getElementById("sessionRestoreContainer").hidden = true;
>+  }
>+});

In a previous review Gavic suggested setting container to doc.getElementById(....) which you did (attachment #540671 [details] [diff] [review]). Let's do that again so we don't call getElementById twice.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-28 13:24:21 PDT
I've pushed the r+ed patch with the tweak I suggested to fx-team:
http://hg.mozilla.org/integration/fx-team/rev/109894baf0c7

It'll make its way on to mozilla-central (and into Nightly) when fx-team gets merged next (hopefully within a week!).

Thanks again for the patch, Alastair!
Comment 25 Tim Taubert [:ttaubert] 2011-07-30 21:42:17 PDT
http://hg.mozilla.org/mozilla-central/rev/109894baf0c7

Note You need to log in before you can comment on or make changes to this bug.