Open Bug 1414024 Opened 7 years ago Updated 2 years ago

bypassCache propertie should not be ignored for an discarded tab when using tabs.reload

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox57 wontfix)

Status:
ASSIGNED
Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: kernp25, Assigned: kernp25)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

bypasscache.diff
(deleted), patch
u462496
: feedback-
Details | Diff | Splinter Review
Bug 1414024 - Implement bypass cache on restore for SHIP. r?mixedpuppy
(deleted), text/x-phabricator-request
Details
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.
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
status-firefox57: --- → wontfix
Priority: -- → P5
Attached patch bypasscache.diff (deleted) — — Splinter Review
What do you think of this?
Attachment #8933007 - Flags: feedback?(kevinhowjones)
Attachment #8933007 - Flags: feedback?(bob.silverberg)
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.
Attachment #8933007 - Flags: feedback?(bob.silverberg)
(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
(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
(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?
Flags: needinfo?(kernp25)
(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?
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
Flags: needinfo?(kernp25)
You can test it with this page: http://www.t-online.de/
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
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);
(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.
Attachment #8933007 - Flags: feedback?(kevinhowjones) → feedback-
(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.
Product: Toolkit → WebExtensions

Can this be implemented?

Flags: needinfo?(rob)

The original bug report did not have enough detail to allow people who are unfamiliar with extensions to reproduce the problem. Here are the minimal steps-to-reproduce:

Minimal STR:

  1. Install https://addons.mozilla.org/en-US/firefox/addon/discard-tab/
  2. Start a simple server to see if requests are being handled, e.g. python -m http.server 8080 or (if using Python 2) python -m SimpleHTTPServer 8080
  3. Visit http://localhost:8080/
  4. Switch to a different tab.
  5. Right-click on the tab from step 3 and click on "Discard Tab"
  6. Inspect the background page of that add-on (via about:debugging) to get access to a place to run extension code.
  7. Get the tabId for the tab from step 3, e.g. tabId = await browser.tabs.query({url: "http://localhost:8080/*"})[0]
  8. Reload the tab: browser.tabs.reload(tabId, {bypassCache:true})
  9. Look at the console from step 2.

Expected:

  • Two requests with GET / (i.e. the one from step 3 and from step 8)

Actual:

  • Only one request with GET / (from step 3; step 8 is missing)

(In reply to kernp25 from comment #14)

Can this be implemented?

I see that you've proposed a patch three years ago. Could you look at the current code and propose a patch? I'll guide you through the development of a proper bugfix.

From a quick look, I see that the reload logic is here: https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/browser/base/content/tabbrowser.js#2189-2203

There is an attempt to restore the tab before passing on the reloadWithFlags call. I would expect the reload to be interpreted as a fresh reload when reloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE) is used. I suggest to look in that direction - what can you do to avoid the initial restore and skip straight ahead to loading the content anew.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rob)

(In reply to Rob Wu [:robwu] from comment #16)

I see that you've proposed a patch three years ago. Could you look at the current code and propose a patch? I'll guide you through the development of a proper bugfix.

I will try. Is there a meta bug or something that can link to this bug?
So other people will find this bug and maybe also want to start working on this?

Flags: needinfo?(rob)

The current behavior is a bug. Because the bypassCache property will be completely ignored for an discarded tab.

(In reply to kernp25 from comment #17)

I will try. Is there a meta bug or something that can link to this bug?
So other people will find this bug and maybe also want to start working on this?

There's generally no need to link bugs for more visibility. All it does is causing more emails to be sent.
Bug 1462813 does however look like a good meta bug to link to, in case subscribers are interested in this bug.

(In reply to kernp25 from comment #19)

The current behavior is a bug. Because the bypassCache property will be completely ignored for an discarded tab.

That's clear. Reproduction steps are in comment 15.

Blocks: tab-unloading
Flags: needinfo?(rob)

You worked on https://bugzilla.mozilla.org/show_bug.cgi?id=675539

Maybe you can fix this bug?

Flags: needinfo?(gsvelto)

The code in the patch is from my old xul addon.

Most of the code, that needs to be changed, is in ContentRestore.jsm.

Maybe you can fix this bug or find someone, who can?

In my old xul add-on i was doing this, to bypass the cache from a discarded tab:

history.QueryInterface(Ci.nsIWebNavigation).reload(reloadFlags);

I don't know, if this still works to.

Flags: needinfo?(gsvelto) → needinfo?(mconley)

I am not familiar with the internal mechanisms used to restore tabs unfortunately, sorry.

I'm afraid I don't currently have the cycles to look at this. Perhaps someone on the WebExtension team might, though.

Flags: needinfo?(mconley)

This is a P5 bug, which means that it is not a high priority for the WebExtensions team, compared to the work on th lot of other bugs/features that we have.

If you feel that this is a high priority, then you are welcome to submit a patch. As said before, I am willing to provide mentorship on resolving this bug. There is a guide for contributing code to the WebExtensons API implementation at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

(In reply to Rob Wu [:robwu] from comment #16)

From a quick look, I see that the reload logic is here: https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/browser/base/content/tabbrowser.js#2189-2203

There is an attempt to restore the tab before passing on the reloadWithFlags call. I would expect the reload to be interpreted as a fresh reload when reloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE) is used. I suggest to look in that direction - what can you do to avoid the initial restore and skip straight ahead to loading the content anew.

I have looked at it again and my idea would be:
To add back the reloadFlags that where removed in Bug 1534638.

So we can use the flags in restoreTabContent, to decide, if the load should bypass the cache.

What do you think?

If yes, a new bug should be created to re-add the reloadFlags?

Flags: needinfo?(rob)

I don't see much value in a new bug about adding something if there is no concrete gain from making the change.

If you think that making the change fixes this issue, submit a patch with a unit test. If the patch looks fine, doesn't cause regressions and the new test passes (while the test failed before the fix), then we can land the patch and mark this bug as resolved.

Flags: needinfo?(rob)
Assignee: nobody → kernp25
Status: NEW → ASSIGNED
Attachment #9275613 - Attachment description: Bug 1414024 - WIP: Implement bypass cache on restore for SHIP. r?robwu → Bug 1414024 - Implement bypass cache on restore for SHIP. r?robwu
Attachment #9275613 - Attachment description: Bug 1414024 - Implement bypass cache on restore for SHIP. r?robwu → Bug 1414024 - WIP: Implement bypass cache on restore for SHIP. r?robwu
Attachment #9275613 - Attachment description: Bug 1414024 - WIP: Implement bypass cache on restore for SHIP. r?robwu → Bug 1414024 - Implement bypass cache on restore for SHIP. r?mixedpuppy
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: