1065478 - Regression: POSTs were coming from offline application cache instead of network

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

We broke the condition for loading from appcache, that has to satisfy responses only for GET or similar methods.  Hence POST must go to the network.

How this has happen:
- when the old cache is used, we assemble the cache v1 cache key that also consist of a POST id
- it is then attempted to convert to URL which fails and the complete appcache choice fails
=> we go to the network

The new cache however tries to find the entry only by the URL, so it passes, cache entry is found and loaded from appcache.

This is easy to fix, we may simply add a condition to bypass appcache lookup/load when the method is not "GET" or "HEAD".

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

It's OK to test for mPostID only since it's non-null for POST and we never run this code for anything else then GET or HEAD otherwise.

Comment 2

[Jason Duell]

Honza, so the regression here started in Firefox 31 or 32?  I would have thought 32, but we had email from someone saying 31 was broken as well.  Also, am I correct in thinking that reverting to cache1 will not actually fix things (since this is nsHttpChannel code not the cache internals that we can switch between)?

sicking: Do you know who we can contact to make sure this doesn't break any B2G apps or websites?  I'm not sure how common it is to POST from an appcache app to a URI that happens to be in the appcache (but must nevertheless hit the network, since it's a POST: that's my understanding of the bustage scenario here).

Comment 3

[Jason Duell]

Sites that serve their login UI with /LOGIN (from the appcache) but then expect a POST to /LOGIN to go to the server are a primary use case here.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #2)
> Honza, so the regression here started in Firefox 31 or 32?  

Only 32, tested.

> I would have
> thought 32, but we had email from someone saying 31 was broken as well. 

Bad info.

> Also, am I correct in thinking that reverting to cache1 will not actually
> fix things (since this is nsHttpChannel code not the cache internals that we
> can switch between)?

It would fix it since the anti-error is actually in the new cache code.  Switching the pref would fix it.

> 
> sicking: Do you know who we can contact to make sure this doesn't break any
> B2G apps or websites?  I'm not sure how common it is to POST from an
> appcache app to a URI that happens to be in the appcache (but must
> nevertheless hit the network, since it's a POST: that's my understanding of
> the bustage scenario here).

Just a note: the thing is that the POST request is performed in a context that is appcached (the channel getd an appcache to use for caching from the associated document).

Comment 5

[Kimon]

The Firefox 31 bug was cache related but different: cache-headers for AppCache manifest file needed no-cache, and max-age=0 stopped working suddenly with F31. Apparently other browsers and pre-31 are more lax in this requirement.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think you could check with Harald Kirschner or Frederic Wenzel.

Comment 7

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/798ea1c2f70c

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/798ea1c2f70c

Comment 9

[Honza Bambas (:mayhemer)]

[Tracking Requested - why for this release]:

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8487320 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: cache2enable
[User impact if declined]: form POSTs on appcached web pages won't be sent to the server (inability to login, purchase goods, etc...) ; this is against the appcache spec
[Describe test coverage new/current, TBPL]: existing appcache functionility is tested with a series of offline application cache tests ; this regression has zero testing coverage since we don't want to add new tests for a dead technology (appcache)
[Risks and why]: zero, this is isolated to appcache usage only, any damage to common web content load would be immediately discovered
[String/UUID change made/needed]: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c12a3c6193b3
https://hg.mozilla.org/releases/mozilla-beta/rev/6c39ccb686a5

Comment 13

[Kimon]

thanks for fixing - since HTML5 AppCache is referred to as a dead technology above, what do you recommend we look into as a replacement?

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Kimon from comment #13)
> thanks for fixing - since HTML5 AppCache is referred to as a dead technology
> above, what do you recommend we look into as a replacement?

Service Workers.  It's being implemented in Gecko now.