Closed Bug 1065478 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking: Cache, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file)

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".
Attached patch v1Splinter Review
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.
Attachment #8487320 - Flags: review?(jduell.mcbugs)
Attachment #8487320 - Flags: review?(jduell.mcbugs) → review+
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).
Flags: needinfo?(honzab.moz)
Summary: POSTs are coming from offline application cache → Regression: POSTs were coming from offline application cache instead of network
Flags: needinfo?(jonas)
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.
(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).
Flags: needinfo?(honzab.moz)
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.
I think you could check with Harald Kirschner or Frederic Wenzel.
Flags: needinfo?(jonas)
https://hg.mozilla.org/mozilla-central/rev/798ea1c2f70c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
[Tracking Requested - why for this release]:
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
Attachment #8487320 - Flags: approval-mozilla-beta?
Attachment #8487320 - Flags: approval-mozilla-aurora?
Attachment #8487320 - Flags: approval-mozilla-beta?
Attachment #8487320 - Flags: approval-mozilla-beta+
Attachment #8487320 - Flags: approval-mozilla-aurora?
Attachment #8487320 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1069280
thanks for fixing - since HTML5 AppCache is referred to as a dead technology above, what do you recommend we look into as a replacement?
(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.
You need to log in before you can comment on or make changes to this bug.