Closed Bug 491168 Opened 12 years ago Closed 11 years ago

Allow SessionStore to save/restore referrer field

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: morac, Assigned: morac)

References

()

Details

Attachments

(1 file, 1 obsolete file)

The referrer field for documents in tabs is currently not saved nor restored.  This includes the case where a tab is closed and then recovered via the "Recently Closed Tabs" menu .  It would be nice for this field to be saved in the case where a page is not cached and the page checks the referrer field before allowing the page to load.

As the document.referrer field is read only, this is not something that can be fixed in nsSessionStore.js alone.

Test case:

Go to URL and click on link on the page, close tab, reopen tab and refresh page.  Referrer is lost.
(In reply to comment #0)
> As the document.referrer field is read only, this is not something that can be
> fixed in nsSessionStore.js alone.

nsIWebNavigation (at least) has an argument to set the referrer field, not sure if this can be used in sessionstore though...
(In reply to comment #0)
> As the document.referrer field is read only, this is not something that can be
> fixed in nsSessionStore.js alone.

The referrer can be set through the nsISHEntry interface which SessionStore already uses for (de)serializing history entries. This should be a three-line patch (and a few dozen lines for a test case).

Want to give it a try, Michael?
(In reply to comment #2)
> The referrer can be set through the nsISHEntry interface which SessionStore
> already uses for (de)serializing history entries. This should be a three-line
> patch (and a few dozen lines for a test case).
> 
> Want to give it a try, Michael?

Okay.  I added the following to _serializeHistoryEntry after the storing of the ID:

    if (aEntry.referrerURI)
      entry.referrerURI = aEntry.referrerURI;

and the following to _deserializeHistoryEntry after the restoring of the contentType:

    if (aEntry.referrerURI)
      shEntry.referrerURI = aEntry.referrerURI;


This appears to work in the case of closing and opening tabs, but if I try to restart Minefield or load a saved session on the nightly trunk load, Minefield crashes on "browser.webNavigation.gotoIndex(activeIndex);" in restoreHistory:

http://crash-stats.mozilla.com/report/index/979afebe-87ee-462c-b49e-3c6c82090503


In that case aEntry.referrerURI is defined as an object instead of an instance of a nsiURL so that's probably why it's crashing.  The crash should probably be filed as a bug in any case since it results when messing with the referrerURI in session history, but I can't come up with a method of recreating it without modifying nsSessionStore.js.


In any case I need to find some way of taking the aEntry.referrerURI and converting it into an instance of nsiURI (and making sure it's valid so it doesn't cause a crash).  Any ideas?
Assignee: nobody → morac99-firefox
Status: NEW → ASSIGNED
(In reply to comment #3)
>     if (aEntry.referrerURI)
>       entry.referrerURI = aEntry.referrerURI;

For serializing, you rather want the aEntry.referrerURI.spec

> The crash should probably be filed as a bug in any case

Indeed, please do so.

> In any case I need to find some way of taking the aEntry.referrerURI and
> converting it into an instance of nsiURI

That's what ioService.newURI should do (which we already use for restoring the visited URLs themselves).
(In reply to comment #4)
> (In reply to comment #3)
> >     if (aEntry.referrerURI)
> >       entry.referrerURI = aEntry.referrerURI;
> 
> For serializing, you rather want the aEntry.referrerURI.spec
> 

I went to post that I had changed it to the following which works, but collided with your post.
shEntry.referrerURI = ioService.newURI(aEntry.referrerURI.spec, null, null);

So apparently I figured this out correctly.

> > The crash should probably be filed as a bug in any case
> 
> Indeed, please do so.
> 

The problem I'm having is that I don't know how to reproduce this problem without making changes to nsSessionStore.js.  I could file it without a test case, but it would be better if it had one.
I filed the crash as bug 491204, feel free to comment on it if you have any additional insight.

Now I just need to get a test case working.  I'm assuming we'd want to test actually saving and restoring a session and not just closing and restoring a tab or window, correct?
(In reply to comment #5)
> shEntry.referrerURI = ioService.newURI(aEntry.referrerURI.spec, null, null);

You should get the spec already at serializing, so that we don't carry any nsIURIs around internally.

(In reply to comment #6)
> I'm assuming we'd want to test actually saving and restoring a session and
> not just closing and restoring a tab or window, correct?

What about:
1. Load a page with a referrer.
2. Make sure that referrer is saved for getTabState.
3. Set a different referrer through setTabState.
4. Ensure the page's document.referrer has been correctly changed.
5. Bonus points for closing/reopening or duplicating the tab.

That should sufficiently test the added code path. No need for saving/restoring a whole session...
(In reply to comment #7)
> You should get the spec already at serializing, so that we don't carry any
> nsIURIs around internally.

Yeah, I realized that was kind of stupid and had already changed it after posting.

> What about:
> 1. Load a page with a referrer.
> 2. Make sure that referrer is saved for getTabState.
> 3. Set a different referrer through setTabState.
> 4. Ensure the page's document.referrer has been correctly changed.
> 5. Bonus points for closing/reopening or duplicating the tab.

Okay I got a test that does all this finished and working.  It took me a lot longer than it should have, but it took me a while to figure out how to delay between the steps to give time for the referrer to propagate.

I'll upload shortly.
Attached patch Patch and test files (obsolete) — Splinter Review
Attachment #375557 - Flags: review?(zeniko)
Comment on attachment 375557 [details] [diff] [review]
Patch and test files

Looking good.

>+    if (aEntry.referrerURI)
>+      entry.referrerURISpec = aEntry.referrerURI.spec;

Please use referrerUrl or even just referrer instead of the overlong referrerURISpec.

>+ * The Initial Developer of the Original Code is
>+ * Simon Bünzli <zeniko@gmail.com>.

Wouldn't that be you?

>+  const REFER1 = "http://www.example.net/?" + Date.now(), REFER2 = "http://www.example.net/?" + Math.random();

Nit: Please complete the names and make it one variable declaration per line.

>+  browser.addEventListener("load", function loadListener(e) {

If you want to explicitly list the function's argument, please name it aEvent.

>+    ss.setTabState(tab, JSON.stringify(tabState));
>+
>+    executeSoon(function() {

That looks too fragile. Please wait for the SSTabRestored notification.

>+  browser.loadURI("http://www.example.net", referrerURI, null);

Since you don't need referrerURI before, please don't declare it until right above this line.

r+=me with those changes.
Attachment #375557 - Flags: review?(zeniko)
(In reply to comment #10)
> Please use referrerUrl or even just referrer instead of the overlong
> referrerURISpec.

Okay I'll change that.

> Wouldn't that be you?

I originally copied the test code from another test that you wrote, but ended up completely rewriting it so, yeah I guess it should be me.

> Nit: Please complete the names and make it one variable declaration per line.

What exactly do you mean by "complete the names"?
Do you mean use "REFERRER" instead of "REFER"?

> If you want to explicitly list the function's argument, please name it aEvent.

Done

> That looks too fragile. Please wait for the SSTabRestored notification.

Done

> Since you don't need referrerURI before, please don't declare it until right
> above this line.

Okay
(In reply to comment #11)
> Do you mean use "REFERRER" instead of "REFER"?

Yes.
Made requested changes.  Submitting for superreview.  Is that even necessary for this patch?
Attachment #375557 - Attachment is obsolete: true
Attachment #375684 - Flags: superreview?(dietrich)
Attachment #375684 - Flags: review+
Attachment #375684 - Flags: superreview?(dietrich)
Comment on attachment 375684 [details] [diff] [review]
Patch and test files 2.0

In browser code, superreview is only needed for API changes.
Ah okay then.

Since the patch is so trivial and since I filed the bug on behalf of a few Session Manager users, I'd like if it could make it into Firefox 3.5.  I'm not sure what the procedure for doing so is exactly.  Do I just "?" the wanted-firefox3.5 flag or do I "?" the approval1.9.1 flag?  Does it have to land on the trunk before I do so?
Keywords: checkin-needed
(In reply to comment #15)
> I'd like if it could make it into Firefox 3.5.

The patch will first have to land and "bake" on Trunk. If no issues emerge (i.e. if the tree remains green and there are no perf regressions), you can set the approval1.9.1? flag and summarize for the branch drivers, why this patch should still be taken at this late point in the game.
Attachment #375684 - Flags: review+ → review?(zeniko)
Keywords: checkin-needed
Attachment #375684 - Flags: review?(zeniko) → review+
Keywords: checkin-needed
Target Milestone: --- → Firefox 3.6a1
It's been a while since I tagged this "checkin-needed". Any chance this could pushed to the Trunk?
(In reply to comment #17)
> It's been a while since I tagged this "checkin-needed". Any chance this could
> pushed to the Trunk?

Since this is nothing for Firefox 3.5 you'll have to wait till the restriction for mozilla-central is over. Please see <http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/15a349fecbfaf701/f414ce5077ff01a4> for further information.
Ah okay I didn't realize there was a freeze (though it didn't start until 2 weeks after I originally requested a check-in).
http://hg.mozilla.org/mozilla-central/rev/0c5edb1d5db1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 507605
Blocks: 524369
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.