1198387 - Setting the network.http.use-cache pref to false breaks fetch interception

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[(no longer active)]

This causes gHttpHandler->UseCache() to return false, bypassing OpenCacheEntry and SW interception.  We need to fix this somehow.

This also means that when we move CORS preflights to happen inside Necko, setting this pref to false will bypass CORS preflights.  Not so great!

Patrick, can we remove support for this pref?

Comment 1

[Patrick McManus [:mcmanus]]

yes - that can be removed. It was  used to test the efficacy of the cache on android at one point.

Comment 2

[Josh Matthews [:jdm]]

We should get rid of the pref from http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#1255 , and get rid of all the code in nsHttpHandler.cpp that makes use of it.

Comment 3

[Kalpesh Krishna [:martianwars]]

(In reply to Josh Matthews [:jdm] from comment #2)
> We should get rid of the pref from
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#1255 , and get rid of all the code in nsHttpHandler.cpp that makes use of
> it.

Hey Josh, could I try my hand at this bug? Excited to try out something new.

Comment 4

[Josh Matthews [:jdm]]

Go for it! Please ask questions if anything is unclear!

Comment 5

[(no longer active)]

Note that you also need to remove code that references this pref, such as nsHttpChannel::mUseCache and its usages.

Comment 6

[Kalpesh Krishna [:martianwars]]

Attachment: Removing user-cache preference to solve bug.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 8654595 [details] [diff] [review]
Removing user-cache preference to solve bug.

Review of attachment 8654595 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like what I expected, so good work :) That being said, did you actually compile again after making these changes? Please remove nsHttpHandler::UseCache and then ask :mcmanus for review, as I am not a peer of the network code (https://wiki.mozilla.org/Modules/Core#Necko)

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ -173,5 @@
>      , mLegacyAppVersion("5.0")
>      , mProduct("Gecko")
>      , mCompatFirefoxEnabled(false)
>      , mUserAgentIsDirty(true)
> -    , mUseCache(true)

Not sure how this patch compiles without getting rid of the method from nsHttpHandler.h, too.

Comment 8

[Kalpesh Krishna [:martianwars]]

(In reply to Josh Matthews [:jdm] from comment #7)
> Comment on attachment 8654595 [details] [diff] [review]
> Removing user-cache preference to solve bug.
> 
> Review of attachment 8654595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like what I expected, so good work :) That being said, did you
> actually compile again after making these changes? Please remove
> nsHttpHandler::UseCache and then ask :mcmanus for review, as I am not a peer
> of the network code (https://wiki.mozilla.org/Modules/Core#Necko)
> 
> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ -173,5 @@
> >      , mLegacyAppVersion("5.0")
> >      , mProduct("Gecko")
> >      , mCompatFirefoxEnabled(false)
> >      , mUserAgentIsDirty(true)
> > -    , mUseCache(true)
> 
> Not sure how this patch compiles without getting rid of the method from
> nsHttpHandler.h, too.

Yeah I forgot about the header file :/ Have removed them now. Would a simple ./mach build be sufficient to compile the program?

Comment 9

[Kalpesh Krishna [:martianwars]]

Attachment: remove_cache.patch

Comment 10

[Josh Matthews [:jdm]]

Yes, `./mach build`, or `./mach build binaries` (if you've only modified .cpp and .h files) will be a quicker build.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8654933 [details] [diff] [review]
remove_cache.patch

Review of attachment 8654933 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

bug comment should have r=mcmanus on the same line as "Bug 119.." and it has user as a typo for use

please, check the try run I will attach below. (the sheriff will want to see it to land the code)

Comment 12

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=e8221453787e

Comment 13

[Kalpesh Krishna [:martianwars]]

Yeah the test run seems to have completed successfully. Should I resubmit the patch?

Comment 14

[Josh Matthews [:jdm]]

Yes please! You can then go ahead and add the "checkin-needed" keyword to the Keywords field up top, and it will be committed to the official tree.

Comment 15

[Kalpesh Krishna [:martianwars]]

Attachment: remove_cache.patch

Patch statement edited, r=mcmanus added.

Comment 16

[Kalpesh Krishna [:martianwars]]

(In reply to Josh Matthews [:jdm] from comment #14)
> Yes please! You can then go ahead and add the "checkin-needed" keyword to
> the Keywords field up top, and it will be committed to the official tree.

Is it alright? I wasn't sure about the review field so added a + there. If I need to re-review it from :mcmanus or you do let me know.

Comment 17

[(no longer active)]

No, that is all you needed to do.  Thanks!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bef4367e19

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2bef4367e19