Closed Bug 1248049 Opened 5 years ago Closed 5 years ago

Don't HTTP cache XHR POSTs

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

AFAIK we cache POSTs only to allow back/forth caching and resend.  Nothing more.  XHR POST response will never be reused from cache, I think.

Can we set INHIBIT_CACHING to spare memory, disk spin and CPU?

Could somewhat help with bug 1215970.
Flags: needinfo?(bzbarsky)
Note that POST response cannot be reused unless nsICacheInfoChannel.cacheKey is explicitly set to the proper internal unique POST id.  Only doc shell and nsWebBrowserPersist do that.
I was going to say "we already do that".  But it looks like the code I remembered us having got removed in bug 1176988, which totally should have gotten review from a necko peer.  :(

Do we just want to reinstate that codepath with the INHIBIT_CACHING flag (but without the LOAD_BYPASS_CACHE flag, I guess)?
Flags: needinfo?(bzbarsky)
Anyway, this is clearly a new regression in 43... :(
Blocks: 1176988
Keywords: regression
Attached patch v1 (obsolete) — Splinter Review
Should not be a problem to pass also LOAD_BYPASS_CACHE since in case of possible interception we still open a cache entry from synthetic storage (open truncate).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee1b8b5b1464
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8719035 - Flags: review?(josh)
Attached patch v1.1 (obsolete) — Splinter Review
The test failure

TEST-UNEXPECTED-FAIL | /XMLHttpRequest/setrequestheader-header-allowed.htm | XMLHttpRequest: setRequestHeader() - headers that are allowed (Pragma) - assert_equals: expected "pragma," but got ""

expectancy has been removed with bug 1176988.  As this is actually a partial backout of that bug, this has to be readded as well.

Otherwise, the patch doesn't cause any failures!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=667bc0782388
Attachment #8719035 - Attachment is obsolete: true
Attachment #8719035 - Flags: review?(josh)
Attachment #8719456 - Flags: review?(josh)
Better try run (w/o bug 1247644, that is not strictly necessary to fix SCtC problem):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07dc7aa24bad
> As this is actually a partial backout of that bug, this has to be readded as well.

Well, hold on.  That means we're adding an explicit spec violation that we had removed.  Can we please avoid doing that?  A followup is probably OK, as long as we actually fix things....
OK, followup is necessary because we need to quickly fix a top crash/high jank bug.
Depends on: 1248613
(In reply to Boris Zbarsky [:bz] from comment #7)
> > As this is actually a partial backout of that bug, this has to be readded as well.
> 
> Well, hold on.  That means we're adding an explicit spec violation that we
> had removed.  Can we please avoid doing that?  A followup is probably OK, as
> long as we actually fix things....

Filed bug 1248613.
Attached patch v2Splinter Review
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1248613#c4

using LOAD_BYPASS_LOCAL_CACHE that does the job we want and doesn't reintroduce the regression.
Attachment #8719456 - Attachment is obsolete: true
Attachment #8719456 - Flags: review?(josh)
Attachment #8719841 - Flags: review?(josh)
Attachment #8719841 - Flags: review?(josh) → review+
Thanks Josh!
Keywords: checkin-needed
Comment on attachment 8719841 [details] [diff] [review]
v2

[Approval Request Comment]

Bug caused by (feature/regressing bug #): bug 1176988

User impact if declined: major slowdown in overall responsiveness, shutdown hanging when using certain combination of favorite addons (Skype Click to Call) and various favorite A/V software, see bug 1215970 comment 98 and 100

Testing completed: try runs, local testing, just landed

Risk to taking this patch (and alternatives if risky): I believe very very low, we had similar (not identical tho!) behavior before bug 1176988 (i.e. in Fx42)

String or UUID changes made by this patch: none
Attachment #8719841 - Flags: approval‑mozilla‑b2g44?
Attachment #8719841 - Flags: approval-mozilla-release?
Attachment #8719841 - Flags: approval-mozilla-beta?
Attachment #8719841 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/651afdf7400c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Duplicate of this bug: 1215970
Comment on attachment 8719841 [details] [diff] [review]
v2

Hopefully fix the famous Skype crash... Taking it.
Should be in 45 beta 7
Attachment #8719841 - Flags: approval-mozilla-beta?
Attachment #8719841 - Flags: approval-mozilla-beta+
Attachment #8719841 - Flags: approval-mozilla-aurora?
Attachment #8719841 - Flags: approval-mozilla-aurora+
Comment on attachment 8719841 [details] [diff] [review]
v2

No longer relevant
Attachment #8719841 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8719841 [details] [diff] [review]
v2

Apparently, no one wants this patch on that old B2G.
Attachment #8719841 - Flags: approval‑mozilla‑b2g44?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.