Closed Bug 1165269 Opened 5 years ago Closed 4 years ago

Use origin for http cache

Categories

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

defect

Tracking

()

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox41 --- affected
firefox44 --- fixed

People

(Reporter: allstars.chh, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

In Bug 1163254 we will update the origin for the new security model. So for http cache it should use origin instead of using appId/isBrowser, for example, in nsILoadContextInfo.idl and nsIHttpAuthManager.idl.
nsILoadContextInfo carries more than what load context carries.  nsILoadContextInfo is also intended for use in the networking code where nsIPrincipal is not available (or used not to be avail)
Also, there are helper functions that covert principals to load context info.
No longer depends on: 1163254
Blocks: 1179985
Private thread with Jonas:

On Wed, Jul 8, 2015 at 10:26 AM, Honza Bambas <honzab.moz@firemni.cz> wrote:
> On 7/8/2015 19:16, Jonas Sicking wrote:
>>
>> On Wed, Jul 8, 2015 at 9:31 AM, Honza Bambas <honzab.moz@firemni.cz>
>> wrote:
>>>>
>>>> Is there a reason that's a .idl (nsILoadContextInfo)
>>>> interface rather than just a plain c++ class? Do we need to expose it
>>>> to JS anywhere?
>>>>
>>>> / Jonas
>>>
>>> Yes, it has to be exposed to JS, there are addons that need to work with
>>> the
>>> cache (open entries) and to do that they need nsILoadContextInfo.  That
>>> interface is mandatory to enforce proper isolation of
>>> private/non-private/app/anonymous/whatever data jars.
>>>
>>> -hb-
>>
>> Ok, that should actually not be a problem since OriginAttributes is
>> also scriptable. See for example
>>
>> http://mxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#173
>> http://mxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#255
>>
>> / Jonas
>>
>
> Sounds good.  Are bugs https://bugzilla.mozilla.org/show_bug.cgi?id=1165269
> and https://bugzilla.mozilla.org/show_bug.cgi?id=1165256 the right place to
> do this work?
>
> -hb-

Yes. I suspect we'll also need a bug for cookies if there's not one already.

Please do make sure that all patches are looked at by Bobby since
using this stuff right isn't entirely obvious yet.

/ Jonas
Assignee: nobody → honzab.moz
Blocks: nsec-origins
I noticed that two simultaneously (at least very close) requests would be made with different app id, which results some strange behavior of the packaged app service. For example, open the browser, for the recently visited page, search app and system app will make a request to get the favicon at a time. I kind of don't even know the expected behavior since one URI can only has one cache file for one app id.

I assume this bug will change the cache hash key to origin (scheme+hostname+port) only so we will have no worry about the case I mentioned.

Valentin, do you think I am correct or it shouldn't be an issue? Thanks!
Flags: needinfo?(valentin.gosu)
(In reply to Henry Chang [:henry] from comment #4)
> I noticed that two simultaneously (at least very close) requests would be
> made with different app id, which results some strange behavior of the
> packaged app service. For example, open the browser, for the recently
> visited page, search app and system app will make a request to get the
> favicon at a time. I kind of don't even know the expected behavior since one
> URI can only has one cache file for one app id.

I'm not sure I undestand the issue. From what I know on desktop, the favicon service (which is not part of necko) tries to get the favicon for whichever page you visit using the system principal. So if you visit http://example.com/package.pak!//index.html it will try and fetch http://example.com/favicon.ico
I also see two requests for the favicon, but since they don't go through the packaged app service, this should not be a problem. It could be different on mobile.

> Valentin, do you think I am correct or it shouldn't be an issue? Thanks!

I don't think this should be an issue. If you still have concerns about it, I'm happy to listen (maybe move the discussion to another bug?)
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #5)
> (In reply to Henry Chang [:henry] from comment #4)
> > I noticed that two simultaneously (at least very close) requests would be
> > made with different app id, which results some strange behavior of the
> > packaged app service. For example, open the browser, for the recently
> > visited page, search app and system app will make a request to get the
> > favicon at a time. I kind of don't even know the expected behavior since one
> > URI can only has one cache file for one app id.
> 
> I'm not sure I undestand the issue. From what I know on desktop, the favicon
> service (which is not part of necko) tries to get the favicon for whichever
> page you visit using the system principal. So if you visit
> http://example.com/package.pak!//index.html it will try and fetch
> http://example.com/favicon.ico
> I also see two requests for the favicon, but since they don't go through the
> packaged app service, this should not be a problem. It could be different on
> mobile.
> 
> > Valentin, do you think I am correct or it shouldn't be an issue? Thanks!
> 
> I don't think this should be an issue. If you still have concerns about it,
> I'm happy to listen (maybe move the discussion to another bug?)

Sorry it's not favicon but other similar background request to download icon.

BTW, I got some misunderstanding about the cache. Let me figure it out a lot more. Thanks!
I think I got what's happening. I filed Bug 1195713 and let's discuss in that bug. Thanks!
Jumping on this bug now.
Status: NEW → ASSIGNED
Is it OK if we with this change blow away data cached for apps (i.e. appid > 0)?  Same applies to appcache.  Cached entries will simply not be found.
Flags: needinfo?(bobbyholley)
Attached patch wip1 (backup) (obsolete) — Splinter Review
If it's just cached data, I think it's probably ok. But let's double-check with Jonas.
Flags: needinfo?(bobbyholley) → needinfo?(jonas)
I actually need first bug 1168777 be solved to do a full migration here.
No longer blocks: 1168777
Depends on: 1168777
Attached patch wip2 (backup) (obsolete) — Splinter Review
Attachment #8652829 - Attachment is obsolete: true
Depends on: 1199775
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #12)
> I actually need first bug 1168777 be solved to do a full migration here.
Hi Honza
We have already sent 'clear-origin-data' notification.
So you should listen to this message in CacheObserver instead of listening to webapps-clear-data.

Therefore we could remove the webapps-clear-data one by one then remove mozIApplicationClear... in Bug 1168777.
Priority: -- → P2
Target Milestone: --- → FxOS-S8 (02Oct)
Attached patch v1 (obsolete) — Splinter Review
This is the first version that builds on Win but is blocked on further development (actually running it and testing) on [1].  It's close to be submitted for first review round but since I cannot test it at all, asking for feedback so far.

Review will anyway be needed (also) from Michal as he knows this code.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c81
Attachment #8654310 - Attachment is obsolete: true
Attachment #8655555 - Flags: feedback?(allstars.chh)
Comment on attachment 8655555 [details] [diff] [review]
v1

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

Should we also listen to clear-origin-data in netwerk/protocol/http/nsHttpAuthCache.cpp and netwerk/protocol/http/nsHttpAuthCache.cpp ?

::: netwerk/base/LoadContextInfo.h
@@ +7,5 @@
>  
>  #include "nsILoadContextInfo.h"
>  
>  class nsIChannel;
> +class nsILoadInfo;

I don't understand why you change to nsILoadInfo here.

@@ +31,4 @@
>  };
>  
> +LoadContextInfo*
> +GetLoadContextInfo(nsIChannel *aChannel);

nit, * sticks with the type.
nsIChannel*

Most of the Necko the * sticks to the variable.
But the original style of this file sticks to the type.
So just a reminder here. :P

::: netwerk/base/nsILoadContextInfo.idl
@@ +16,5 @@
>   * It shall be used where nsILoadContext cannot be used or is not
>   * available.
>   */
> +
> +[scriptable, uuid(555e2f8a-a1f6-41dd-88ca-ed4ed6b98a22)]

should we make this builtin-class ?

::: netwerk/cache/nsApplicationCacheService.cpp
@@ +56,5 @@
>  
>  NS_IMETHODIMP
>  nsApplicationCacheService::BuildGroupIDForApp(nsIURI *aManifestURL,
>                                                uint32_t aAppId,
>                                                bool aIsInBrowser,

I guess you will change to use OriginAttributes in the app_cache bug 1165256

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2818,5 @@
>    if (aLoadContextInfo) {
>      LOG(("  anonymous=%u, inBrowser=%u, appId=%u]",
>          aLoadContextInfo->IsAnonymous(),
> +        aLoadContextInfo->OriginAttributesRef()->mInBrowser,
> +        aLoadContextInfo->OriginAttributesRef()->mAppId));

Maybe we could print originSuffix here.

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +1044,5 @@
>    NS_ENSURE_TRUE(info, NS_ERROR_FAILURE);
>  
>    mAnonymous =  info->IsAnonymous();
> +  mAppId = info->OriginAttributesRef()->mAppId;
> +  mInBrowser = info->OriginAttributesRef()->mInBrowser;

Should we store OriginAttributes instead of mAppId/mInBrowser?

::: netwerk/cache2/CacheObserver.cpp
@@ -395,5 @@
>  
> -class CacheStorageEvictHelper
> -{
> -public:
> -  nsresult Run(mozIApplicationClearPrivateDataParams* aParams);

#include "mozIApplicationClearPrivateDataParams.h" should be removed as well.

@@ +518,5 @@
> +  if (!strcmp(aTopic, "clear-origin-data")) {
> +    OriginAttributes oa;
> +    if (!oa.Init(nsDependentString(aData))) {
> +      NS_ERROR("Could not parse OriginAttributes JSON in clear-origin-data notification");
> +      return NS_OK;

return NS_OK here?
Attachment #8655555 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 8655555 [details] [diff] [review]
v1

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

::: netwerk/base/nsNetUtil.cpp
@@ +1220,5 @@
> +    if (!loadContext) {
> +        return false;
> +    }
> +
> +    loadContext->GetOriginAttributes(aAttributes);

Many Necko test cases will implement nsILoadContext in JS, and calling this on JS-impl nsILoadContext will trigger some error like 'Mismatch Compartment'.
The patch is missing two things I realize:
- update to LoadContextInfo.jsm
- update of the cache file metadata and cache index formats to accept a much more generic origin string (needs some thinking first)

(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #16)
> > +[scriptable, uuid(555e2f8a-a1f6-41dd-88ca-ed4ed6b98a22)]
> 
> should we make this builtin-class ?

I would prefer not.  I want JS be able to implement this easily and not have nsILoadContextInfo be a component.  However under the current circumstances (i.e. work with the OriginAttributes struct being much harder) we may reconsider.

> >  nsApplicationCacheService::BuildGroupIDForApp(nsIURI *aManifestURL,
> >                                                uint32_t aAppId,
> >                                                bool aIsInBrowser,
> 
> I guess you will change to use OriginAttributes in the app_cache bug 1165256

Yes and I mention it there.

> > +      NS_ERROR("Could not parse OriginAttributes JSON in clear-origin-data notification");
> > +      return NS_OK;
> 
> return NS_OK here?

Doesn't matter, right?

(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #17)
> Comment on attachment 8655555 [details] [diff] [review]
> v1
> 
> Review of attachment 8655555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsNetUtil.cpp
> @@ +1220,5 @@
> > +    if (!loadContext) {
> > +        return false;
> > +    }
> > +
> > +    loadContext->GetOriginAttributes(aAttributes);
> 
> Many Necko test cases will implement nsILoadContext in JS, and calling this
> on JS-impl nsILoadContext will trigger some error like 'Mismatch
> Compartment'.

And what do you suggest then?  Actually I have to say that the way how one must work with the OriginAttributes 
struct is limiting and complicated.
Flags: needinfo?(allstars.chh)
So, by looking closer at the code, the disk format of CacheFileMetadata won't need to change.  All the changes are contained in the key which the patch already updates.

However, more changes will be needed in the cache index.  The current format nicely packs the appid (uint32) and the browser flag (1 bit).  But the OriginAttributes is a generic string/generic key+value list.  That cannot be easily packed in a binary way.

Filing a new bug on that.
Blocks: 1201042
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #9)
> Is it OK if we with this change blow away data cached for apps (i.e. appid >
> 0)?  Same applies to appcache.  Cached entries will simply not be found.

Yeah, I think this is fine. It would be nice if we didn't blow away the appcache, but I suspect it's not worth the hassle of migrating the data.
Flags: needinfo?(jonas)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #18)
> > ::: netwerk/base/nsNetUtil.cpp
> > @@ +1220,5 @@
> > > +    if (!loadContext) {
> > > +        return false;
> > > +    }
> > > +
> > > +    loadContext->GetOriginAttributes(aAttributes);
> > 
> > Many Necko test cases will implement nsILoadContext in JS, and calling this
> > on JS-impl nsILoadContext will trigger some error like 'Mismatch
> > Compartment'.
> 
> And what do you suggest then?  Actually I have to say that the way how one
> must work with the OriginAttributes 
> struct is limiting and complicated.

Actually this is the problem I met in Bug 1165466.
just let you know.

I met lots of error from Necko because of this.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #21)
> Actually this is the problem I met in Bug 1165466.
> just let you know.
> 
> I met lots of error from Necko because of this.

Aha.  OK, but we need some solution here.  I'm stuck on delivering the patches in production quality, I can run them, test them, nothing...
(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #21)
> Actually this is the problem I met in Bug 1165466.
> just let you know.
> 
> I met lots of error from Necko because of this.

Actually, why don't you rather do the same thing as me at [1]?  Question of course is how should a JS implementation of that interface deal with the binary version of the getter.  I presume it will never work when a C++ code will call the binary version of an object implemented in JS...

Hmm... have to think.  Maybe nsILoadContextInfo should be a builtin class and somehow be "creatable" from JS...  Like with nsILoadContextInfoFactory...  very nice :)

Or your approach should simply be fixed and used here as well.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8655555&action=diff#a/netwerk/base/nsILoadContextInfo.idl_sec2
comment 23 ^
Flags: needinfo?(allstars.chh)
So, based on bug 1165466 comment 83 I will convert to the same approach as allstars.  Slight API change (a bit more complicated to use) but doable.
Flags: needinfo?(allstars.chh)
No longer depends on: 1168777
Blocks: 1165256
Depends on: 1203979
Blocks: 1168777
Attached patch v2 (obsolete) — Splinter Review
- nsILoadContextInfo made a built-in class, now it has just a single C++ implementation
- introduced nsILoadContextInfoFactory that actually replaces LoadContextInfo.jsm with a C++ impl
- LoadContextInfo.jsm left and simply forwards to the LoadContextInfoFactory service
- some necessary changes are already made to some parts of the appcache code
- listening to "clear-origin-data" instead of "webapps-clear-data" in CacheObserver ; tests fixed according this change too

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b9bab64e51

Michal may want to take a look as well.
Attachment #8655555 - Attachment is obsolete: true
Attachment #8659993 - Flags: review?(michal.novotny)
Attachment #8659993 - Flags: review?(allstars.chh)
I have updated some prerequisite patches, retrying:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc9bb4599c2b
Attached patch v2.1 (obsolete) — Splinter Review
Sorry to obsolete the v2 patch potentially under your hands.  This is an update (not many changes) to make the test work.  Mostly it's just about privileges.

(This patch also includes the deletion of persistent eviction files from disk.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f67713b3e3e
Attachment #8659993 - Attachment is obsolete: true
Attachment #8659993 - Flags: review?(michal.novotny)
Attachment #8659993 - Flags: review?(allstars.chh)
Attachment #8661320 - Flags: review?(michal.novotny)
Attachment #8661320 - Flags: review?(allstars.chh)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #28)
>
> (This patch also includes the deletion of persistent eviction files from
> disk.)

Why is it in this patch and not in patch in bug 1032254?
Flags: needinfo?(honzab.moz)
Comment on attachment 8661320 [details] [diff] [review]
v2.1

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

Looks good to me, except the change in CacheFileContextEvictor.cpp.
Attachment #8661320 - Flags: review?(michal.novotny) → review+
(In reply to Michal Novotny (:michal) from comment #29)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #28)
> >
> > (This patch also includes the deletion of persistent eviction files from
> > disk.)
> 
> Why is it in this patch and not in patch in bug 1032254?

Because I'm starting to be lost among all these bugs :D  will move it there.  Thanks.
Flags: needinfo?(honzab.moz)
Pushed one little change to make this build on linux 64 debug (although it builds for me locally OK): https://treeherder.mozilla.org/#/jobs?repo=try&revision=26d37b463cdb
OK, pushed to static analyzes correctly this time (the reason why locally builds):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a1fd4c069e
Comment on attachment 8661320 [details] [diff] [review]
v2.1

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

I am not a Necko expert, r+ on the OriginAttributes stuff.
Also I noticed some nits.

Thanks

::: netwerk/base/LoadContextInfo.cpp
@@ +37,5 @@
>    *aIsAnonymous = mIsAnonymous;
>    return NS_OK;
>  }
>  
> +OriginAttributes const* LoadContextInfo::OriginAttributesRef()

This looks a little strange to me.
The name is Ref, but you return a pointer here.

@@ +42,5 @@
> +{
> +  return &mOriginAttributes;
> +}
> +
> +NS_IMETHODIMP LoadContextInfo::GetOriginAttributes(JSContext* aCx,

nit, JSContext *aCx

@@ +47,5 @@
> +                                                   JS::MutableHandle<JS::Value> aVal)
> +{
> +  if (NS_WARN_IF(!ToJSValue(aCx, mOriginAttributes, aVal))) {
> +    return NS_ERROR_FAILURE;
> +  }

nit, the pattern is usually
bool ok = ToJSValue(aCx, mOriginAttributes, aVal);
NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);

@@ +81,5 @@
> +}
> +
> +/* nsILoadContextInfo custom (in boolean aPrivate, in boolean aAnonymous, in jsval aOriginAttributes); */
> +NS_IMETHODIMP LoadContextInfoFactory::Custom(bool aPrivate, bool aAnonymous,
> +                                             JS::HandleValue aOriginAttributes, JSContext* cx,

nit, *aCx

@@ +117,1 @@
>  LoadContextInfo *

This is super nit,
you change the style to 'LoadContextInfo*' in the header,
maybe we should do the same thing here.

@@ +117,2 @@
>  LoadContextInfo *
>  GetLoadContextInfo(nsIChannel * aChannel)

same for nsIChannel*

@@ +117,4 @@
>  LoadContextInfo *
>  GetLoadContextInfo(nsIChannel * aChannel)
>  {
> +  nsresult rv;

I am not sure why you move definition of rv here.
Looks to me the original code is better.

@@ +130,3 @@
>  
> +  OriginAttributes oa;
> +  NS_GetOriginAttributes(aChannel, oa);

check return value.

@@ +144,4 @@
>  
>    bool pb = aLoadContext->UsePrivateBrowsing();
> +  OriginAttributes oa;
> +  aLoadContext->GetOriginAttributes(oa);

check return value.

::: netwerk/base/LoadContextInfo.h
@@ +7,5 @@
>  
>  #include "nsILoadContextInfo.h"
>  
>  class nsIChannel;
> +class nsILoadInfo;

Why change to nsILoadInfo?
Don't we still need nsILoadContext in line 45?

::: netwerk/base/nsILoadContextInfo.idl
@@ +41,5 @@
> +   */
> +  [implicit_jscontext]
> +  readonly attribute jsval originAttributes;
> +  [noscript, notxpcom, nostdcall, binaryname(OriginAttributesRef)]
> +  OriginAttributesPtr binaryOriginAttributesRef();

The name is Ref, but you return a pointer here.

::: netwerk/cache/nsApplicationCacheService.cpp
@@ +58,5 @@
>                                                nsACString &_result)
>  {
> +    OriginAttributes oa;
> +    oa.mAppId = aAppId;
> +    oa.mInBrowser = aIsInBrowser;

OriginAttributes oa(aAppId, aIsInBrowser);

And there should be a TODO here for BuildGroupIDForApp to use OriginAttributes, maybe the bug of app_cache, right?

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +2414,5 @@
> +  // nsCacheService::GlobalInstance()->EvictEntriesInternal(nsICache::STORE_OFFLINE);
> +
> +  OriginAttributes oa;
> +  oa.mAppId = appID;
> +  oa.mInBrowser = browserEntriesOnly;

ditto

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +20,5 @@
>  #include "nsClassHashtable.h"
>  #include "nsWeakReference.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Mutex.h"
> +#include "mozilla/BasePrincipal.h"

Could we use forward declarsion for OriginAttributes here?

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2817,5 @@
>  
>    nsresult rv;
>  
>    if (aLoadContextInfo) {
>      LOG(("  anonymous=%u, inBrowser=%u, appId=%u]",

Should print OriginSuffix here.

::: netwerk/cache2/CacheIndex.h
@@ +272,2 @@
>          aInfo->IsAnonymous() == !!(aRec->mFlags & kAnonymousMask) &&
> +        aInfo->OriginAttributesRef()->mInBrowser == !!(aRec->mFlags & kInBrowserMask)) {

I don't know if it makes sense to store OriginAttributes in CacheIndexRecord, but if it does, 
we should compare OriginAttributes directly.

::: netwerk/cache2/CacheObserver.cpp
@@ +113,1 @@
>    NS_ADDREF(sSelf);

#include "mozIApplicationClearPrivateDataParams.h" should also be removed in this file.

::: netwerk/test/mochitests/test_web_packaged_app.html
@@ +42,5 @@
>    }
>  };
>  
>  function checkCacheEntry(url, exists, appId) {
> +  var loadContext = appId ? LoadContextInfo.custom(false, false, {appId: appId, inBrowser: false}) : LoadContextInfo.default;

nit, inBrowser: false could be removed.

::: netwerk/test/unit/test_cache_jar.js
@@ +62,5 @@
>    let procType = Cc["@mozilla.org/xre/runtime;1"].getService(Ci.nsIXULRuntime).processType;
>    if (procType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT)
>      return;
>  
> +  Services.obs.notifyObservers(null, "clear-origin-data", "{\"appId\":1,\"inBrowser\":\"1\"}");

let attrs = {appId: 1, inBrowser: true};
Services.obs.notifyObservers(null, "clear-origin-data", JSON.stringify(attrs));

@@ +72,5 @@
>      yield undefined;
>    }
>  
> +  Services.obs.notifyObservers(null, "clear-origin-data", "{\"appId\":1}");
> +  Services.obs.notifyObservers(null, "clear-origin-data", "{\"appId\":1,\"inBrowser\":\"1\"}");

ditto
Attachment #8661320 - Flags: review?(allstars.chh) → review+
(In reply to Yoshi Huang[:allstars.chh] from comment #35)
> > +OriginAttributes const* LoadContextInfo::OriginAttributesRef()
> 
> This looks a little strange to me.
> The name is Ref, but you return a pointer here.

Yep, thinking of Ptr myself.

> > +  nsresult rv;
> 
> I am not sure why you move definition of rv here.
> Looks to me the original code is better.

I prefer rv be defined as the first and alone in a method/function.  I've seen bugs doing this otherwise.

> 
> @@ +130,3 @@
> >  
> > +  OriginAttributes oa;
> > +  NS_GetOriginAttributes(aChannel, oa);
> 
> check return value.

As you can see in the previous code, on failure we go with defaults.  If it should change, then not in this bug.

> 
> @@ +144,4 @@
> >  
> >    bool pb = aLoadContext->UsePrivateBrowsing();
> > +  OriginAttributes oa;
> > +  aLoadContext->GetOriginAttributes(oa);
> 
> check return value.

ditto

> ::: netwerk/cache2/CacheIndex.h
> @@ +272,2 @@
> >          aInfo->IsAnonymous() == !!(aRec->mFlags & kAnonymousMask) &&
> > +        aInfo->OriginAttributesRef()->mInBrowser == !!(aRec->mFlags & kInBrowserMask)) {
> 
> I don't know if it makes sense to store OriginAttributes in
> CacheIndexRecord, but if it does, 
> we should compare OriginAttributes directly.

Bug 1201042 


Thanks!
(In reply to Yoshi Huang[:allstars.chh] from comment #35)
> > +NS_IMETHODIMP LoadContextInfo::GetOriginAttributes(JSContext* aCx,
> 
> nit, JSContext *aCx

This is actually no longer correct - we fixed this in bug 1144366. :-)
(In reply to Bobby Holley (:bholley) from comment #37)
> (In reply to Yoshi Huang[:allstars.chh] from comment #35)
> > > +NS_IMETHODIMP LoadContextInfo::GetOriginAttributes(JSContext* aCx,
> > 
> > nit, JSContext *aCx
> 
> This is actually no longer correct - we fixed this in bug 1144366. :-)

Wish one day we had |Type * var| form...
(In reply to Bobby Holley (:bholley) from comment #37)
> (In reply to Yoshi Huang[:allstars.chh] from comment #35)
> > > +NS_IMETHODIMP LoadContextInfo::GetOriginAttributes(JSContext* aCx,
> > 
> > nit, JSContext *aCx
> 
> This is actually no longer correct - we fixed this in bug 1144366. :-)

yeah, I told to Honza before.
However it looks most Necko code still uses 'T *t' style, and most of his also use this.
So that was a just reminder for him to use consistent style.
Attached patch v2.2 (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b08810a2cac

Everything should be fixed.

Thanks for the reviews!
Attachment #8661320 - Attachment is obsolete: true
Attachment #8671523 - Flags: review+
I rebased the patch to not depend on pinning and deferring, so that we can land this r+'ed patch right now.

Rather retrying:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c435addadd6
Attachment #8671523 - Attachment is obsolete: true
Attachment #8672261 - Flags: review+
This patch doesn't modify auth cache.  I filed a separate bug 1213577 for it.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c08ebe9b7150
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
What a great work! Thank you Honza!
Comment on attachment 8672261 [details] [diff] [review]
v2.2 (rebased on pure m-c)

Bobby, despite this has already landed, please overlook the changes, mainly the CacheObserver changes [1] that involve handling of clear-origin-data notification.

I'm afraid that the cache, as it's designed from the very start, cannot use pattern matching easily w/o more changes (beyond this bug).  If it is the case, it's depending on changes in the cache index, bug 1201042, and yet adding pattern matching to it as well (a new bug probably needed).


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8672261&action=diff#a/netwerk/cache2/CacheObserver.cpp_sec1


PS: sorry, I somehow forgot to involve you in the review process originally.  Let's do it at least post-facto.
Attachment #8672261 - Flags: review?(bobbyholley)
Comment on attachment 8672261 [details] [diff] [review]
v2.2 (rebased on pure m-c)

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

::: netwerk/cache2/CacheObserver.cpp
@@ +515,5 @@
>    }
>  
> +  if (!strcmp(aTopic, "clear-origin-data")) {
> +    OriginAttributes oa;
> +    if (!oa.Init(nsDependentString(aData))) {

So, this is wrong, because the thing in clear-origin-data is an OriginAttributesPattern, rather than an OriginAttributes. The important distinction between the two is that OriginAttributes will generate default values for any attributes that are not passed, whereas OriginAttributesPattern will match _any_ of the non-passed attributes.

So in ClearStorage above, you won't be clearing all of the storage than needs to be cleared.
Attachment #8672261 - Flags: review?(bobbyholley) → review-
(Given that this landed, fixing it in a followup is totally fine btw)
(In reply to Bobby Holley (:bholley) from comment #47)
> Comment on attachment 8672261 [details] [diff] [review]
> v2.2 (rebased on pure m-c)
> 
> Review of attachment 8672261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheObserver.cpp
> @@ +515,5 @@
> >    }
> >  
> > +  if (!strcmp(aTopic, "clear-origin-data")) {
> > +    OriginAttributes oa;
> > +    if (!oa.Init(nsDependentString(aData))) {
> 
> So, this is wrong, because the thing in clear-origin-data is an
> OriginAttributesPattern, rather than an OriginAttributes. The important
> distinction between the two is that OriginAttributes will generate default
> values for any attributes that are not passed, whereas
> OriginAttributesPattern will match _any_ of the non-passed attributes.
> 
> So in ClearStorage above, you won't be clearing all of the storage than
> needs to be cleared.

Yep, that's what I'm afraid of.  Thanks.  I figured out how exactly clear-origin-data works only today.

Filing a followup.
Blocks: 1214360
Blocks: 1218087
QA Whiteboard: [COM=NSec]
You need to log in before you can comment on or make changes to this bug.