Fennec's reload button seems to be operating in "Shift-Reload" mode <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1718>. The service worker spec mandates the implementation to bypass the service worker, therefore we get into the situation where in air plane mode, I can load "Trained to Thrill" fine, but when I press reload, I get a Server Not Found error because we bypass the service worker. The same problem also exists in the "Request deskop mode" feature: <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3608>

This blocks v1 if we're going to ship on Fennec too.

See also bug 1202052.

Looks like bug 1202052 has more discussion on this...

Comment on attachment 8663360 [details] [diff] [review] Stop bypassing the cache (and service workers) when reloading content in Fennec Dropping r?until we figure out the UX in bug 1202052

I'm un-duping, because I think we should just land this patch on its own, and figure out how to give users back a "shift reload" functionality over in bug 1202052.

Comment on attachment 8663360 [details] [diff] [review] Stop bypassing the cache (and service workers) when reloading content in Fennec Review of attachment 8663360 [details] [diff] [review]: ----------------------------------------------------------------- I think it would be fine to land this and let it bake on Nightly for a bit. We could also make a post to mobile-firefox-dev to ask Nightly testers to keep an eye out for issues with reload functionality.

Thanks, Margaret! I think this unblocks service workers. :-)

Comment on attachment 8663360 [details] [diff] [review] Stop bypassing the cache (and service workers) when reloading content in Fennec Approval Request Comment [Feature/regressing bug #]: This is needed if we want to ship Service Workers on Fennec 43. [User impact if declined]: Comment 0. [Describe test coverage new/current, TreeHerder]: Tested locally. [Risks and why]: Some people were worried in bug 1202052 that this may break people's workflows in ways that we don't expect. AFAIK nobody has filed a bug about that on Nightly yet, but bug 1202052 landing on top of it on Nightly may make the issue less severe on 44. The issue with this patch is that it impacts non-serviceworker related things, so the risk on the uplift needs to be evaluated accordingly. [String/UUID change made/needed]: None.

This is still broken. STR: 1) Open https://people.mozilla.org/~bdahl/sandbox/service_worker_cache/go.html 2) Click Register 3) Use the browser reload button Results: ServiceWorker controller: null Expected: ServiceWorker controller: [object ServiceWorker] If you use the reload button on the page itself it will work correctly. I haven't verified yet, but as I mentioned in the other bug the flags should be LOAD_FLAGS_NONE

Tried it locally, with LOAD_FLAGS_NONE it behaves as expected.

Brendan, I just tried this in 44.0a1 (2015-09-30) and I can't reproduce the failure you describe in comment 15. This was with e10s. What version and platform did you test on?

Oh, this is only fennec. Sorry for my confusion. I was testing nightly. Moving this to block the android ship bug.

Comment on attachment 8663360 [details] [diff] [review] Stop bypassing the cache (and service workers) when reloading content in Fennec Ah you're right Brendan... I'm not sure why I previously thought that this patch fixes anything. Let me submit a new one.

Comment on attachment 8668720 [details] [diff] [review] Use LOAD_FLAGS_NONE unless shift-reload has been requested Review of attachment 8668720 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ -1727,5 @@ > browser.gotoIndex(index); > break; > > case "Session:Reload": { > - let flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY; Shouldn't we move this ... @@ -1734,5 @@ > if (aData) { > let data = JSON.parse(aData); > > if (data.bypassCache) { > flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE; ... to here?

Sounds like bug 1202052 is waiting on this to uplift to aurora. If that's the case, then, when this has a patch ready for uplift, please nominate it and let me know. Thanks!

(In reply to Sebastian Kaspari (:sebastian) from comment #21) > ::: mobile/android/chrome/content/browser.js > @@ -1727,5 @@ > > browser.gotoIndex(index); > > break; > > > > case "Session:Reload": { > > - let flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY; > > Shouldn't we move this ... > > @@ -1734,5 @@ > > if (aData) { > > let data = JSON.parse(aData); > > > > if (data.bypassCache) { > > flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE; > > ... to here? Sure.

Comment on attachment 8671049 [details] [diff] [review] Use LOAD_FLAGS_NONE unless shift-reload has been requested Approval Request Comment See comment 14, please.

Ioana, can your team verify the fix here and in bug 1202052 on Nightly? Then we can uplift the patches from both bugs at some point next week.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #25) > Comment on attachment 8671049 [details] [diff] [review] > Use LOAD_FLAGS_NONE unless shift-reload has been requested > > Approval Request Comment > See comment 14, please. I'm not sure if we can uplift this? The patch is based on changes done in the previous (pushed but not uplifted) patch and some changes I did in bug 1202052 (also not uplifted yet). Maybe we should create a combined patch of the previous patch here, the new patch here and the changes in bug 1202052?

(In reply to Sebastian Kaspari (:sebastian) from comment #29) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #25) > > Comment on attachment 8671049 [details] [diff] [review] > > Use LOAD_FLAGS_NONE unless shift-reload has been requested > > > > Approval Request Comment > > See comment 14, please. > > I'm not sure if we can uplift this? The patch is based on changes done in > the previous (pushed but not uplifted) patch and some changes I did in bug > 1202052 (also not uplifted yet). Maybe we should create a combined patch of > the previous patch here, the new patch here and the changes in bug 1202052? Bug 1202052 is waiting for approval. Once it gets it, we can uplift this on top of it, no? (If you meant the patches not applying cleanly etc, I can land it on Aurora manually.)

(In reply to Ehsan Akhgari (don't ask for review please) from comment #30) > Bug 1202052 is waiting for approval. Once it gets it, we can uplift this on > top of it, no? > > (If you meant the patches not applying cleanly etc, I can land it on Aurora > manually.) Okay! I was just concerned about us having landed multiple patches here and one in bug 1202052, but only 2 uplift approval requests. :)

Comment on attachment 8671049 [details] [diff] [review] Use LOAD_FLAGS_NONE unless shift-reload has been requested OK to uplift along with its companion patch in bug 1202052. This blocks service workers for 43.

Hi ehsan, this cause problems when uplifting to aurora: merging mobile/android/chrome/content/browser.js warning: conflicts during merge. merging mobile/android/chrome/content/browser.js incomplete! (edit conflicts, then use 'hg resolve --mark') could you take a look, thanks!