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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Bill Walker [:bwalker] [@wfwalker]]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 2

[Andrew Overholt [:overholt]]

Is this related to the recent Necko changes in this area (bug 654579)?

Comment 3

[Brendan Dahl [:bdahl]]

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.

Comment 4

[Brendan Dahl [:bdahl]]

I'm getting my load flags mixed up. 'LOAD_RELOAD_NORMAL' is a docshell LoadType. The flags should just be nsIWebNavigation::LOAD_FLAGS_NONE

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I guess snorp is not needed :)

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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.

Comment 9

[Brendan Dahl [:bdahl]]

> 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.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 13

[(no longer active)]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 14

[:Margaret Leibovic]

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?

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #14)
> Sebastian, could you look into adding this new long tap functionality to the
> reload button?

Yep!

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1202052 - Bypass cache on reload button long press. r?margaret

Bug 1202052 - Bypass cache on reload button long press. r?margaret

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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 21

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(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!

Comment 23

[:Margaret Leibovic]

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

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/631931fb2658

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1202052-aurora-uplift.patch

This is the version of the patch I pushed (for requesting uplift).

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Just saw that we did not uplift bug 1206458. But this bug here has status-firefox43:affected set.

Comment 28

[(no longer active)]

(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?

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/631931fb2658

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

(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?

Comment 31

[(no longer active)]

(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.

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

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]: -

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

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 35

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Ioana can someone from your team verify, once this lands?  Thanks!

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d8441167722