1248049 - Don't HTTP cache XHR POSTs

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Anyway, this is clearly a new regression in 43... :(

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

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

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1.1

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

Comment 6

[Honza Bambas (:mayhemer)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Honza Bambas (:mayhemer)]

OK, followup is necessary because we need to quickly fix a top crash/high jank bug.

Comment 9

[Honza Bambas (:mayhemer)]

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

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v2

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.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8719841 [details] [diff] [review]
v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cd1d4f528a4

Comment 12

[Honza Bambas (:mayhemer)]

Thanks Josh!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/651afdf7400c

Comment 14

[Honza Bambas (:mayhemer)]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/651afdf7400c

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8719841 [details] [diff] [review]
v2

Hopefully fix the famous Skype crash... Taking it.
Should be in 45 beta 7

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b1a3d50e474b

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/93a38fee8b43

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8719841 [details] [diff] [review]
v2

No longer relevant

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8719841 [details] [diff] [review]
v2

Apparently, no one wants this patch on that old B2G.