The reload button breaks service workers

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 44
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox43+ fixed, firefox44+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → ehsan
See also bug 1202052.
Status: NEW → ASSIGNED
Looks like bug 1202052 has more discussion on this...
No longer blocks: ServiceWorkers-v1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1202052
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
Attachment #8663360 - Flags: review?(mark.finkle)
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.
Status: RESOLVED → REOPENED
Depends on: 1202052
Resolution: DUPLICATE → ---
Blocks: 1202052
No longer depends on: 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.
Attachment #8663360 - Flags: review+
Thanks, Margaret!  I think this unblocks service workers.  :-)
https://hg.mozilla.org/mozilla-central/rev/802679c08c17
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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.
Attachment #8663360 - Flags: approval-mozilla-aurora?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Flags: needinfo?(bdahl)
Oh, this is only fennec.  Sorry for my confusion.  I was testing nightly.  Moving this to block the android ship bug.
Blocks: ServiceWorkers-Android
No longer blocks: ServiceWorkers-v1
Flags: needinfo?(bdahl)
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.
Attachment #8663360 - Flags: approval-mozilla-aurora?
Attachment #8668720 - Flags: review?(margaret.leibovic)
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.
Attachment #8668720 - Attachment is obsolete: true
Attachment #8668720 - Flags: review?(margaret.leibovic)
Attachment #8671049 - Flags: review?(margaret.leibovic)
Attachment #8663360 - Attachment is obsolete: true
Attachment #8671049 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8671049 [details] [diff] [review]
Use LOAD_FLAGS_NONE unless shift-reload has been requested

Approval Request Comment
See comment 14, please.
Attachment #8671049 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8c3b3ff67448
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(ioana.chiorean)
(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?
Flags: needinfo?(ehsan)
(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.)
Flags: needinfo?(ehsan)
(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.
Attachment #8671049 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
You need to log in before you can comment on or make changes to this bug.