kernp25 Assignee
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171102100041



Actual results:

"tabs.reload({bypassCache: true});" does not bypass the cache. The "bypassCache" propertie will be ignored.


Expected results:

"tabs.reload({bypassCache: true});" should also bypass the cache of an discarded tab.

	kernp25 Assignee
	Comment 1 • 7 years ago

If an discarded/pending tab gets restored, "history.reloadCurrentEntry();" will be called [1].

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/ContentRestore.jsm#264

	kernp25 Assignee
	Comment 2 • 7 years ago

What do you think of this?

	Bob Silverberg [:bsilverberg]
	Comment 3 • 7 years ago

Comment on attachment 8933007 [details] [diff] [review]
bypasscache.diff

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

I made a couple of comments on the change to ext-tabs.js, but I don't really know the TabStateCache code, so best to get someone else's feedback on that.

::: browser/components/extensions/ext-tabs.js
@@ +515,5 @@
>            let nativeTab = getTabOrActive(tabId);
>  
>            let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
>            if (reloadProperties && reloadProperties.bypassCache) {
> +            if (!nativeTab.linkedPanel) { // handle discarded tab

Is this the standard way of identifying a discarded tab? Also, we don't tend to use inline comments like this. Probably best to put it on its own line.

	kernp25 Assignee
	Comment 4 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> Comment on attachment 8933007 [details] [diff] [review]
> bypasscache.diff
> 
> Review of attachment 8933007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I made a couple of comments on the change to ext-tabs.js, but I don't really
> know the TabStateCache code, so best to get someone else's feedback on that.
> 
> ::: browser/components/extensions/ext-tabs.js
> @@ +515,5 @@
> >            let nativeTab = getTabOrActive(tabId);
> >  
> >            let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> >            if (reloadProperties && reloadProperties.bypassCache) {
> > +            if (!nativeTab.linkedPanel) { // handle discarded tab
> 
> Is this the standard way of identifying a discarded tab? Also, we don't tend
> to use inline comments like this. Probably best to put it on its own line.

I have this from here:
https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browser.js#596

	u462496
	Comment 5 • 7 years ago

(In reply to kernp25 from comment #2)
> Created attachment 8933007 [details] [diff] [review]
> bypasscache.diff
> 
> What do you think of this?

@@ -509,16 +511,21 @@ this.tabs = class extends ExtensionAPI {

           let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
           if (reloadProperties && reloadProperties.bypassCache) {
+            if (!nativeTab.linkedPanel) { // handle discarded tab
+              TabStateCache.update(nativeTab.linkedBrowser, {
+                bypassCache: true,
+              });
+            }
             flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
           }
           nativeTab.linkedBrowser.reloadWithFlags(flags);

I am wondering why we are having to mess around with TabStateCache internals from an extension API?

I could be wrong, but my gut feeling about this is that we have a bug somewhere internally, and that should be identified.  This seems like it might be more of a workaround to the actual problem to me.  Calling browser.reloadWithFlags should instantiate the browser[1] (make the tab not discarded any longer) and (in theory) should behave as if we were calling reloadWithFlags on a non-discarded tab.

If this is not working the way it is expected for some reason, I think we should be dealing with it internally, rather than from within the extension API.

I'll take a look and see if anything jumps out at me.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2411-2422

	u462496
	Comment 6 • 7 years ago

(In reply to kernp25 from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101
> Firefox/58.0
> Build ID: 20171102100041
> 
> 
> 
> Actual results:
> 
> "tabs.reload({bypassCache: true});" does not bypass the cache. The
> "bypassCache" propertie will be ignored.
> 
> 
> Expected results:
> 
> "tabs.reload({bypassCache: true});" should also bypass the cache of an
> discarded tab.

Can you provide a detailed STR to reproduce this problem?

	u462496
	Comment 7 • 7 years ago

(In reply to Kevin Jones from comment #6)

> Can you provide a detailed STR to reproduce this problem?

eg, how are you verifying whether a page has been loaded from the cache or not?

	kernp25 Assignee
	Comment 8 • 7 years ago

1) Load a news page (a page that is updated every minutes).
2) Discard tab.
3) Wait 10-30 minutes
4) Load the tab (e.g. by clicking on the tab).

Actual results:

The page will have the same content as before the unload (discarding). 

Expected results:

Only a reload will load the updated page.

Btw, i have taken the code for the reload from here [1].

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#69

	kernp25 Assignee
	Comment 9 • 7 years ago

You can test it with this page: http://www.t-online.de/

	kernp25 Assignee
	Comment 10 • 7 years ago

Or maybe we handle it here [1]?

Because now the reloadFlags will be ignored.
Maybe we check here [1] if the flags have LOAD_FLAGS_BYPASS_CACHE and then handle the reload correctly (e.g. calling "history.QueryInterface(Ci.nsIWebNavigation).reload(reloadFlags);" instead of "history.reloadCurrentEntry();" [2]?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/ContentRestore.jsm#422
[2] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/ContentRestore.jsm#264

	kernp25 Assignee
	Comment 11 • 7 years ago

You can also use this code in Scratchpad to reload a page using reloadCurrentEntry:
var tabMM = gBrowser.mCurrentBrowser.frameLoader.messageManager;
tabMM.loadFrameScript("data:,docShell.QueryInterface(Components.interfaces.nsIWebNavigation).sessionHistory.reloadCurrentEntry()", false);

	u462496
	Comment 12 • 7 years ago

(In reply to kernp25 from comment #8)
> 1) Load a news page (a page that is updated every minutes).
> 2) Discard tab.
> 3) Wait 10-30 minutes
> 4) Load the tab (e.g. by clicking on the tab).
> 
> Actual results:
> 
> The page will have the same content as before the unload (discarding). 
> 
> Expected results:
> 
> Only a reload will load the updated page.
> 
> Btw, i have taken the code for the reload from here [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-
> content.js#69
> 
> (and comments 9 - 11)

I'm a bit confused here, first of all, I'm not seeing anything in your STR that has to do with tabs.reload.

And also, what you state here are the results I would expect.  You click on a discarded tab, and the page from the cache appears, just as if it weren't discarded, it would not reload just by navigating to that tab.  Then if you manually reload, it reloads.  What is not expected here?  This is the same behavior as when you close out your browser and restart, the tabs come up in discarded state, and are loaded from the cache when selected.

	kernp25 Assignee
	Comment 13 • 7 years ago

(In reply to Kevin Jones from comment #12)
> (In reply to kernp25 from comment #8)
> > 1) Load a news page (a page that is updated every minutes).
> > 2) Discard tab.
> > 3) Wait 10-30 minutes
> > 4) Load the tab (e.g. by clicking on the tab).
> > 
> > Actual results:
> > 
> > The page will have the same content as before the unload (discarding). 
> > 
> > Expected results:
> > 
> > Only a reload will load the updated page.
> > 
> > Btw, i have taken the code for the reload from here [1].
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-
> > content.js#69
> > 
> > (and comments 9 - 11)
> 
> I'm a bit confused here, first of all, I'm not seeing anything in your STR
> that has to do with tabs.reload.
> 
> And also, what you state here are the results I would expect.  You click on
> a discarded tab, and the page from the cache appears, just as if it weren't
> discarded, it would not reload just by navigating to that tab.  Then if you
> manually reload, it reloads.  What is not expected here?  This is the same
> behavior as when you close out your browser and restart, the tabs come up in
> discarded state, and are loaded from the cache when selected.

Yes, i know that this is the expected behavior, but i created this bug to allow WebExtensions to bypass the cache directly when using tabs.reload({bypassCache: true});

Sorry for the confusion.