Closed Bug 644998 Opened 13 years ago Closed 13 years ago

Session should not be restorable after "Clear Recent History"

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 8

People

(Reporter: blacklion8888, Assigned: alastair)

References

()

Details

(Keywords: privacy, Whiteboard: [good first bug][mentor=zpao])

Attachments

(1 file, 5 obsolete files)

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
Doesn't sound like private browsing.
Component: Private Browsing → General
QA Contact: private.browsing → general
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.
Severity: major → enhancement
Component: General → Session Restore
QA Contact: general → session.restore
Summary: After cleaning -all- temp data, a session restore is available / cleaning temp data disfunction → Session should not be restorable after "Clear Recent History"
Yea we should probably clear that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=zpao]
Version: unspecified → Trunk
Does this bug need some automated tests to be created for it?
Attachment #536500 - Flags: review?(paul)
Comment on attachment 536500 [details] [diff] [review]
Patch 1 - clear this._lastSessionState

Thanks Alastair!
Attachment #536500 - Flags: review?(paul) → review+
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.
Attachment #536500 - Flags: ui-review?(faaborg)
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?
Attachment #536500 - Flags: ui-review?(faaborg) → ui-review+
(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.
>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).
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?
Attachment #536500 - Attachment is obsolete: true
Attachment #538976 - Flags: review?(paul)
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.
Attachment #538976 - Flags: review?(paul) → feedback+
Attached patch Patch 3 - responded to feedback (obsolete) — Splinter Review
Attachment #538976 - Attachment is obsolete: true
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)
Attachment #540671 - Flags: review?(gavin.sharp)
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.
Attachment #540671 - Flags: review?(gavin.sharp) → review+
Attachment #540671 - Attachment is obsolete: true
Attachment #546011 - Flags: review?(gavin.sharp)
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).
Yes, I'll definately have a go at doing this, although I probably will need to contact you on IRC at some point.
Comment on attachment 546011 [details] [diff] [review]
Patch 4 - changed to use forEach()

Excellent! I'll cancel this review request in the mean time.
Attachment #546011 - Flags: review?(gavin.sharp)
Attachment #546011 - Attachment is obsolete: true
Attachment #547479 - Flags: review?(gavin.sharp)
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.
Attachment #547479 - Flags: review?(gavin.sharp) → review+
Attached patch Patch 6Splinter Review
Attachment #547479 - Attachment is obsolete: true
Attachment #549203 - Flags: review?(gavin.sharp)
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.
Attachment #549203 - Flags: review?(gavin.sharp)
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!
Assignee: nobody → alastairrob
http://hg.mozilla.org/mozilla-central/rev/109894baf0c7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Blocks: 675493
Keywords: privacy
You need to log in before you can comment on or make changes to this bug.