716012 - Loading tabs on demand: always load wyciwyg:// URIs right away

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Philipp von Weitershausen [:philikon]]

wyciwyg URIs are explicit pointers to the page cache, IIRC, so not only is it fine to load those right away, we should do it to avoid a nasty wyciwyg:// URI in the tab title.

(BarTab did this :p. See https://github.com/philikon/BarTab/blob/master/modules/prototypes.js#L659)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

1. we don't currently restore wyciwyg pages that are part of a frameset... bug 450595
2. when was the last time you saw a wyciwyg @ top level? (you can do that right?) I'm guessing recently because you're reporting this (example?), but how about before today? :)
3. but fine (the whitelist with about:blank makes a lot of sense too)

So a lot of the time we're going to load your other pages from cache anyway... so avoiding network traffic isn't the only benefit of restore on demand. You could shove a big JS heavy page in cache & loading it hurts. But you knew that.

So given the rarity, I'm not sure how much of a win this would be. Not that we shouldn't do it... but I'll leave it for a [good first bug].

Implementors:
here's the quick idea:

if (selected || current entry uri schemeis("wyciwyg") || current entry uri == "about:blank")
  restoretab

it's probably ok to keep it in restoreHistory, but feel free to pull the logic out into a helper function if you think that makes sense.

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Paul O'Shannessy [:zpao] from comment #1)
> 1. we don't currently restore wyciwyg pages that are part of a frameset...
> bug 450595
> 2. when was the last time you saw a wyciwyg @ top level? (you can do that
> right?) I'm guessing recently because you're reporting this (example?), but
> how about before today? :)

I've noticed that tabs opened from Gmail often have wyciwyg:// URIs.

Comment 3

[Bellindira Castillo [:bellindira]]

Attachment: Always load about:blank and wyciwyg URIs right away

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 619111 [details] [diff] [review]
Always load about:blank and wyciwyg URIs right away

BarTab did it first! :p

Comment 5

[Bellindira Castillo [:bellindira]]

Yes, I only was integrating it to sessionstore because it says:

Implementors:
here's the quick idea:

if (selected || current entry uri schemeis("wyciwyg") || current entry uri == "about:blank")
  restoretab

So, was this something that should be fixed or not?

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 619111 [details] [diff] [review]
Always load about:blank and wyciwyg URIs right away

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

This looks fine, but holding off on review until you can confirm it's working/add a test

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3169,5 @@
> +    // Always load about:blank and wyciwyg URIs right away
> +    // even when restoreOnDemand is true.
> +    if (aWindow.gBrowser.selectedBrowser == browser ||
> +        browser.currentURI.spec == "about:blank" ||
> +        browser.currentURI.scheme == "wyciwyg") {

I'm not 100% sure this is going to be correct with cascaded session restore/overwriting tabs (I think it is, but not 100%!).

Can you first run the session restore tests to make sure they pass, but it may also be good to restore a wyciwyg url into a tab that had normal history and make sure it doesn't kick off a load.

Comment 7

[Bellindira Castillo [:bellindira]]

Yes, I'll do it. BTW, the patch passed the session restore tests.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 619111 [details] [diff] [review]
Always load about:blank and wyciwyg URIs right away

(clearing review while waiting for the test)

Comment 9

[Bellindira Castillo [:bellindira]]

Attachment: Patch v2 and test case.

Added a test to confirm that the patch works.
The patch contains two fixes:
- Restoring a wyciwyg was not working => The browser location after reload was still wyciwyg://#/. So, the patch was fixed to look into sessionHistory now and load the previous URL on history (with a scheme different from wyciwyg). Because on normal conditions, that would be the same URI that wyciwyg is pointing. I think another solution could be to remove wyciwyg://# from the URI and load the fixed URL. 
- Also,"about:blank" is not restored right away when user has typed something on the browser location.

Thanks!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bellindira Castillo [:bellindira] from comment #9)
> - Restoring a wyciwyg was not working => The browser location after reload
> was still wyciwyg://#/. So, the patch was fixed to look into sessionHistory
> now and load the previous URL on history (with a scheme different from
> wyciwyg). Because on normal conditions, that would be the same URI that
> wyciwyg is pointing. I think another solution could be to remove wyciwyg://#
> from the URI and load the fixed URL.

What exactly do you mean by 'was not working'? I see that your test doesn't really restore tabs but somehow loads the first URI and then sets the tabState. Maybe that's the culprit?

I think you should create a whole browser state object and then call waitForBrowserState(). waitForBrowserState() also needs to be adjusted to this patch:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/head.js#55

The callback passed to waitForBrowserState() should then check if all 'about:blank' and 'wyciwyg' tabs have been restored immediately.

> - Also,"about:blank" is not restored right away when user has typed
> something on the browser location.

Good catch.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 624610 [details] [diff] [review]
Patch v2 and test case.

Clearing review until the question has been answered and the test has been fixed.

Comment 12

[Bellindira Castillo [:bellindira]]

Attachment: Patch updated to current code and suggested fixes were applied

(In reply to Tim Taubert [:ttaubert] from comment #10)
> (In reply to Bellindira Castillo [:bellindira] from comment #9)
> > - Restoring a wyciwyg was not working => The browser location after reload
> > was still wyciwyg://#/. So, the patch was fixed to look into sessionHistory
> > now and load the previous URL on history (with a scheme different from
> > wyciwyg). Because on normal conditions, that would be the same URI that
> > wyciwyg is pointing. I think another solution could be to remove wyciwyg://#
> > from the URI and load the fixed URL.
> 
> What exactly do you mean by 'was not working'? I see that your test doesn't
> really restore tabs but somehow loads the first URI and then sets the
> tabState. Maybe that's the culprit?


No. The reason why restoring the wyciwyg scheme was not working  was that the browser didn't do anything different to handle this case. Browser should restore it to a non-wyciwyg scheme. So, the proposal is to restore the tab to its previous URL on history because that's the right state.

If you want to test this "manually" you can do the following: 
1. Change your browser preferences to "Show my windows and tabs from last time".
2. Load this location on your latest build "file:///YOUR PATH/mozilla-central/browser/components/sessionstore/test/browser_716012.html"
3. Hit reload (or Command + R) multiple times until you see wyciwyg:// scheme in the address bar.
4. Close the browser and restart it again. With the fix it should load "file://...", without the fix it loads "wycywg://...". 

> 
> I think you should create a whole browser state object and then call
> waitForBrowserState(). waitForBrowserState() also needs to be adjusted to
> this patch:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/test/head.js#55
> 
> The callback passed to waitForBrowserState() should then check if all
> 'about:blank' and 'wyciwyg' tabs have been restored immediately.

I changed as this proposal. However, on the callback is not possible to check directly which tabs are restored (this is done indirectly, setting the number of tabs that must be restored on head.js) . 
Also, I had to modify the timeout from '0' to '10000' on the file "browser_716012.html" (<body onload="setTimeout(test, 10000);">) which maybe is not the best but the only way I found to make it work ('test' method on timeout messes up with the SSTabRestore. ). Notice waitForBrowserState handles SSTabRestored using the tabContainer and in the previous testcase I add/remove a listener for each single tab.
 
> > - Also,"about:blank" is not restored right away when user has typed
> > something on the browser location.

Yes, I think this is how it is expected to work. Because if you restore about:blank, what the user typed is lost and the location is set to about:blank instead of the location the user has typed.

Thanks.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 646652 [details] [diff] [review]
Patch updated to current code and suggested fixes were applied

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

We should figure out if we still want that bug fixed.

Comment 14

[Andrew McCreight [:mccr8]]

wyciwyg is going away in bug 1489308