Fennec reload button bypasses service workers by doing a "shift reload" unconditionally

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: bwalker, Assigned: sebastian)

Tracking

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

Firefox Tracking Flags

(firefox43+ fixed, firefox44+ fixed)

Details

()

Attachments

(2 attachments)

steps to reproduce:

1. In about:config, set these properties to true, "dom.serviceWorkers.enabled", "dom.serviceWorkers.interception.enabled", and "dom.serviceWorkers.interception.opaque.enabled"
2. Load an HTTPS page containing offlining via serviceworkers (example attached)
3. put android device in airplane mode
4. reload page from step 2

expected results:

web console shows console.log output from serviceworker registration function
page loads from serviceworker cache

actual results:

no console.log output from registration function
"server not found" page.

sample page:
https://wfwalker.github.io/opensun/#latitude=37.741531587322896&longitude=-122.25501118944668&zoom=13
We need to figure out if this is a problem with airplane mode not making Gecko networking go offline, or if it's a problem with service workers.

Sending to snorp since he has looked into networking in the past.
Flags: needinfo?(snorp)
Is this related to the recent Necko changes in this area (bug 654579)?
This seems to be an issue with how fennec does reloads. I noticed that when I just navigate to a page with service workers in airplane mode that the offline page works correctly, but if I use the reload button the page does not load correctly. This got me thinking it was similar to bug 1202895 and it is.

On reload fennec triggers the "Session:Reload" event which does http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1721 . The flags "Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY | Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;" are set which is equivalent to a force reload(shift+reload on desktop) which means gecko will not attach the service worker to the page so there's no chance of us hitting the cache for the page.

We need to change fennec to use the same flags as desktop for reload which should be:
LOAD_RELOAD_NORMAL

Changing to the above will mean there's no way to do a force reload, so we need some way trigger one on fennec.
I'm getting my load flags mixed up. 'LOAD_RELOAD_NORMAL' is a docshell LoadType. The flags should just be nsIWebNavigation::LOAD_FLAGS_NONE
(In reply to Brendan Dahl [:bdahl] from comment #3)
> Changing to the above will mean there's no way to do a force reload, so we
> need some way trigger one on fennec.

My first thought is to expose force-reload via a long press on the reload button, but a Fennec guy like mfinkle should weigh in on that.
We originally switched to the forced-reload approach in bug 835241.

If we decide to switch back, we should consider Myk's idea of using some other approach to allow a forced-reload. NI'ing Anthony to consider UI approaches.

Brendan - I guess the forced-reload thing part of the service worker spec or something?
Flags: needinfo?(alam)
I guess snorp is not needed :)
Flags: needinfo?(snorp)
(In reply to Mark Finkle (:mfinkle) from comment #6)
> We originally switched to the forced-reload approach in bug 835241.
> 
> If we decide to switch back, we should consider Myk's idea of using some
> other approach to allow a forced-reload. NI'ing Anthony to consider UI
> approaches.
> 
> Brendan - I guess the forced-reload thing part of the service worker spec or
> something?

http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm step 12.1 of the handle fetch algorithm.
> Brendan - I guess the forced-reload thing part of the service worker spec or
> something?

Yes, http://www.w3.org/TR/service-workers/#navigator-service-worker-controller . I may be mistaken but I think this is the only spec that has a specific behavior for force reload. Even the forceReload param for window.location.reload() isn't standardized.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> We originally switched to the forced-reload approach in bug 835241.
> 
> If we decide to switch back, we should consider Myk's idea of using some
> other approach to allow a forced-reload. NI'ing Anthony to consider UI
> approaches.
> 
> Brendan - I guess the forced-reload thing part of the service worker spec or
> something?

Long-pressing to do a force-reload isn't the worst thing in the World. That would be my alternative but I would suggest also a small vibrate when it's triggered. So, there's actually a difference and indication that I've held "long enough". 

Although, I don't have the context behind why we switched this to the default action so I can't say how many problems changing this would cause.
Flags: needinfo?(alam)
I'm a little worried that changing our menu item to do a normal reload will cause usability issues. At one point we had telemetry that suggested people use it a *lot*, presumably to work around busted network or cache invalidation problems. We do better on the network issues these days (detecting interface changes), but I still think we should be very cautious.

I'd be more ok with doing a normal reload iff the page has a SW registered. I guess we'd still want to add a way to do a force reload in that case...ugh.

Updated

4 years ago
Duplicate of this bug: 1206458

Comment 13

4 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11)
> I'm a little worried that changing our menu item to do a normal reload will
> cause usability issues. At one point we had telemetry that suggested people
> use it a *lot*, presumably to work around busted network or cache
> invalidation problems. We do better on the network issues these days
> (detecting interface changes), but I still think we should be very cautious.

Hmm, presumably we are worrying about two specific cases: servers sending buggy cache headers to mobile only, or bugs in our cache implementation which only affect mobile and not desktop?  The latter sounds unlikely to me.  Do we have bug reports indicating the former?

> I'd be more ok with doing a normal reload iff the page has a SW registered.
> I guess we'd still want to add a way to do a force reload in that case...ugh.

It looks to me that whatever solution we come up with, we need to be consistent about it.  The user is not going to know whether the page has a SW or not, so doing things differently based on that seems suboptimal to me.
No longer blocks: ServiceWorkers-v1

Updated

4 years ago
Summary: serviceworker does not register with android phone in airplane mode → Fennec reload button bypasses service workers by doing a "shift reload" unconditionally
I think we should just land the patch from bug 1206458 to fix the immediate problem, and then we can use this bug to figure out how to give users back the "shift reload" functionality.

I'd be in favor of trying the proposed long tap solution, and make sure we add in UI telemetry to see how often people actually use this.

By landing bug 1206458 sooner rather than later, we'd also get feedback sooner if this breaks users' reload expectations.

Sebastian, could you look into adding this new long tap functionality to the reload button?
Flags: needinfo?(s.kaspari)

Updated

4 years ago
Blocks: 1206458

Updated

4 years ago
No longer blocks: 1206458
Depends on: 1206458

Updated

4 years ago
No longer blocks: ServiceWorkers-Android
(In reply to :Margaret Leibovic from comment #14)
> Sebastian, could you look into adding this new long tap functionality to the
> reload button?

Yep!
Assignee: nobody → s.kaspari
Flags: needinfo?(s.kaspari)
Status: NEW → ASSIGNED
Note that on Gingerbread we are using the "old" Android menu - and we can't add a long click action to that. I really wish we would have a unified UI (bug 1202076). So what should we do? Ignore Gingerbread?
Bug 1202052 - Bypass cache on reload button long press. r?margaret
Attachment #8665405 - Flags: review?(margaret.leibovic)
The above patch implements bypassing the cache when the reload button is pressed long.

Things to consider:
* Gingerbread devices do not have this button but the old menu thing (No long press)
* Session:Reload is also triggered when the user enables/disables tracking protection on a site - Should this bypass caches? After bug 1206458 it does not anymore.
* Some devices do not vibrate (e.g. Nexus 9) - Should we show a toast additionally?
(In reply to Sebastian Kaspari (:sebastian) from comment #18)
> The above patch implements bypassing the cache when the reload button is
> pressed long.
> 
> Things to consider:
> * Gingerbread devices do not have this button but the old menu thing (No
> long press)

Let's ignore force-reload for Gingerbread

> * Session:Reload is also triggered when the user enables/disables tracking
> protection on a site - Should this bypass caches? After bug 1206458 it does
> not anymore.

Looks like a simple reload is good enough. Desktop uses it here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-trackingprotection.js#152

which calls:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1922

> * Some devices do not vibrate (e.g. Nexus 9) - Should we show a toast
> additionally?

I'd vote to not add it yet, but I could be convinced otherwise.
https://reviewboard.mozilla.org/r/20235/#review18207

::: mobile/android/base/BrowserApp.java:3522
(Diff revision 1)
> +                tab.doReload(true);

Can you add telemetry here?

Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.MENU, "reload_force");
Comment on attachment 8665405 [details]
MozReview Request: Bug 1202052 - Bypass cache on reload button long press. r?margaret

Bug 1202052 - Bypass cache on reload button long press. r?margaret
(In reply to Mark Finkle (:mfinkle) from comment #20)
> Can you add telemetry here?
> 
> Telemetry.sendUIEvent(TelemetryContract.Event.ACTION,
> TelemetryContract.Method.MENU, "reload_force");

Added!

Updated

4 years ago
Attachment #8665405 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8665405 [details]
MozReview Request: Bug 1202052 - Bypass cache on reload button long press. r?margaret

https://reviewboard.mozilla.org/r/20235/#review18371

I assume you tested this on both phones and tablets. I actually just tried long-tapping the reload button on a tablet on Nightly (without this patch), and we currently show the URL bar context menu, which seems like a bug we'll inadvertently fix here.

::: mobile/android/base/BrowserApp.java:3529
(Diff revision 2)
> +        return false;

We also implement this method in GeckoApp [1], so it may be safer to call super.onMenuItemLongClick here in case the GeckoApp implementation ever changes.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#360

Also, it's confusing that we have 2 different interfaces that both have an onMenuItemLongClick method!

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoPopupMenu.java#37
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenu.java#51
(In reply to :Margaret Leibovic from comment #23)
> I assume you tested this on both phones and tablets. I actually just tried
> long-tapping the reload button on a tablet on Nightly (without this patch),
> and we currently show the URL bar context menu, which seems like a bug we'll
> inadvertently fix here.

Yes, tested it on N5 and N9. The weird behavior is now gone. It's still happening when long-pressing the bookmark button though. So we could file a bug for that.

> We also implement this method in GeckoApp [1], so it may be safer to call
> super.onMenuItemLongClick here in case the GeckoApp implementation ever
> changes.

Agreed. I'll update the patch.

> Also, it's confusing that we have 2 different interfaces that both have an
> onMenuItemLongClick method!
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/
> GeckoPopupMenu.java#37
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/
> GeckoMenu.java#51

Yeah. That looks like a good (mentorable) follow-up bug.
This is the version of the patch I pushed (for requesting uplift).
Just saw that we did not uplift bug 1206458. But this bug here has status-firefox43:affected set.

Comment 28

4 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #27)
> Just saw that we did not uplift bug 1206458. But this bug here has
> status-firefox43:affected set.

Yeah, I was waiting for this bug.  :-)  Can we uplift this instead?
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/631931fb2658
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(In reply to Ehsan Akhgari (don't ask for review please) from comment #28)
> Yeah, I was waiting for this bug.  :-)  Can we uplift this instead?

Don't we need to uplift both then? bug 1206458 to make the reload button not bypass cache and this here to add bypassing cache on long-press?
Flags: needinfo?(s.kaspari) → needinfo?(ehsan)

Comment 31

4 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #30)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #28)
> > Yeah, I was waiting for this bug.  :-)  Can we uplift this instead?
> 
> Don't we need to uplift both then? bug 1206458 to make the reload button not
> bypass cache and this here to add bypassing cache on long-press?

Well, people were worried here that bug 1206458 without this bug may break the user's workflow, but I guess I can nominate that on its own.
Flags: needinfo?(ehsan)
Comment on attachment 8666668 [details] [diff] [review]
1202052-aurora-uplift.patch

Approval Request Comment

[Feature/regressing bug #]: This should be uplifted along with bug 1206458.

[User impact if declined]: After uplifting bug 1206458 pressing "reload" will no longer bypass the cache. This patch adds an alternative way of reloading the current page (bypassing the cache).

[Describe test coverage new/current, TreeHerder]: Tested this thoroughly locally with extended logging.

[Risks and why]: Long-pressing the button had no action assigned previously. While this patch touches multiple entry points into reloading the current page, I still consider the risk to be low (The reload method now has an additional parameter to define whether the cache should be bypassed or not).

[String/UUID change made/needed]: -
Attachment #8666668 - Flags: approval-mozilla-aurora?
OK, can you let me know when bug 1206458 lands? If you needinfo me as well or find me on irc when they are both ready for uplift, I can have a look more quickly.
Flags: needinfo?(s.kaspari)
Tracking since this is a blocker for some service workers functionality aimed at Firefox 43. 
It would be to verify this works on nightly before we uplift to aurora.
Comment on attachment 8666668 [details] [diff] [review]
1202052-aurora-uplift.patch

Please uplift this along with the patch in bug 1206458. 
We should verify this fix on aurora.
Attachment #8666668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ioana can someone from your team verify, once this lands?  Thanks!
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
Flags: needinfo?(s.kaspari)
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.