Closed Bug 1198387 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ehsan, Assigned: martianwars, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(mcmanus)
yes - that can be removed. It was  used to test the efficacy of the cache on android at one point.
Flags: needinfo?(mcmanus)
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.
Mentor: josh
Whiteboard: [lang=c++]
(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.
Go for it! Please ask questions if anything is unclear!
Note that you also need to remove code that references this pref, such as nsHttpChannel::mUseCache and its usages.
Attachment #8654595 - Flags: review?(josh)
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.
Attachment #8654595 - Flags: review?(josh) → feedback+
(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?
Attached patch remove_cache.patch (obsolete) — Splinter Review
Attachment #8654933 - Flags: review?(mcmanus)
Yes, `./mach build`, or `./mach build binaries` (if you've only modified .cpp and .h files) will be a quicker build.
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)
Attachment #8654933 - Flags: review?(mcmanus) → review+
Yeah the test run seems to have completed successfully. Should I resubmit the patch?
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.
Assignee: nobody → kalpeshk2011
Patch statement edited, r=mcmanus added.
Attachment #8655303 - Flags: review+
Keywords: checkin-needed
(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.
No, that is all you needed to do.  Thanks!
Attachment #8654595 - Attachment is obsolete: true
Attachment #8654933 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e2bef4367e19
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.