1165269 - Use origin for http cache

Status: RESOLVED FIXED

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

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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.

Comment 1

[Honza Bambas (:mayhemer)]

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)

Comment 2

[Honza Bambas (:mayhemer)]

Also, there are helper functions that covert principals to load context info.

Comment 3

[Honza Bambas (:mayhemer)]

Private thread with Jonas:

On Wed, Jul 8, 2015 at 10:26 AM, Honza Bambas <[e-mail]> wrote:
> On 7/8/2015 19:16, Jonas Sicking wrote:
>>
>> On Wed, Jul 8, 2015 at 9:31 AM, Honza Bambas <[e-mail]>
>> 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

Comment 4

[Henry Chang [:hchang]]

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!

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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

Comment 6

[Henry Chang [:hchang]]

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

Comment 7

[Henry Chang [:hchang]]

I think I got what's happening. I filed Bug 1195713 and let's discuss in that bug. Thanks!

Comment 8

[Honza Bambas (:mayhemer)]

Jumping on this bug now.

Comment 9

[Honza Bambas (:mayhemer)]

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.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: wip1 (backup)

Comment 11

[Bobby Holley (:bholley)]

If it's just cached data, I think it's probably ok. But let's double-check with Jonas.

Comment 12

[Honza Bambas (:mayhemer)]

I actually need first bug 1168777 be solved to do a full migration here.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: wip2 (backup)

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1

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

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 18

[Honza Bambas (:mayhemer)]

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.

Comment 19

[Honza Bambas (:mayhemer)]

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.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 22

[Honza Bambas (:mayhemer)]

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

Comment 23

[Honza Bambas (:mayhemer)]

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

[Honza Bambas (:mayhemer)]

comment 23 ^

Comment 25

[Honza Bambas (:mayhemer)]

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.

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: v2

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

Comment 27

[Honza Bambas (:mayhemer)]

I have updated some prerequisite patches, retrying:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc9bb4599c2b

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v2.1

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

Comment 29

[Michal Novotny [:michal]]

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

Comment 30

[Michal Novotny [:michal]]

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.

Comment 31

[Honza Bambas (:mayhemer)]

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

Comment 32

[Honza Bambas (:mayhemer)]

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

Comment 33

[Honza Bambas (:mayhemer)]

OK, pushed to static analyzes correctly this time (the reason why locally builds):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a1fd4c069e

Comment 34

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12bfd62f9123

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 36

[Honza Bambas (:mayhemer)]

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

Comment 37

[Bobby Holley (:bholley)]

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

Comment 38

[Honza Bambas (:mayhemer)]

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

Comment 39

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 40

[Honza Bambas (:mayhemer)]

Attachment: v2.2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b08810a2cac

Everything should be fixed.

Thanks for the reviews!

Comment 41

[Honza Bambas (:mayhemer)]

Attachment: v2.2 (rebased on pure m-c)

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

Comment 42

[Honza Bambas (:mayhemer)]

This patch doesn't modify auth cache.  I filed a separate bug 1213577 for it.

Comment 43

[Pulsebot]

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

Comment 44

[Carsten Book [:Tomcat]]

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

Comment 45

[Ethan Tseng [:ethan]]

What a great work! Thank you Honza!

Comment 46

[Honza Bambas (:mayhemer)]

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.

Comment 47

[Bobby Holley (:bholley)]

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.

Comment 48

[Bobby Holley (:bholley)]

(Given that this landed, fixing it in a followup is totally fine btw)

Comment 49

[Honza Bambas (:mayhemer)]

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