Provide an entry identifier on nsICacheInfoChannel

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bhsu)

Tracking

32 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [necko-backlog])

Attachments

(3 attachments, 13 obsolete attachments)

6.39 KB, patch
bhsu
: review+
bhsu
: feedback+
Details | Diff | Splinter Review
10.02 KB, patch
bhsu
: review+
bhsu
: feedback+
Details | Diff | Splinter Review
16.81 KB, patch
bhsu
: review+
bhsu
: feedback+
Details | Diff | Splinter Review
In bug 1350359 we would like to be able to tell if two different nsIChannel objects are providing data from the same underlying http cache entry.  Currently there is no way to tell this.

Honza suggests we could relatively easily add a uint64_t identifier that is incremented on every cache entry creation.  If the identifier matches then we can be reasonably assured we are reading from the same cache entry.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Not time critical, someone will take care.  

The implementation idea is to have a static atomic (rel/acq) counter in CacheStorageService and in CacheEntry ctor just grab a ++ value and then expose it on nsICacheEntry as GUII : uint64_t or something, with some good comment.

Thanks.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Whiteboard: [necko-active]
Whiteboard: [necko-backlog]
Assignee: nobody → bhsu
Attachment #8904163 - Attachment is obsolete: true
Attachment #8904164 - Attachment is obsolete: true
Attachment #8905377 - Attachment is obsolete: true
Attachment #8905379 - Attachment is obsolete: true
Attachment #8905380 - Attachment is obsolete: true
Hi Junior,

Thanks for answering many of my questions about Necko. After talking with you today, I decided to make nsICacheInfoChannel::CacheEntryId() only available before onStopRequest as nsICacheInfoChannel::IsFromCache(), and I was wondering whether I can have your feedback before sending them to review?
Flags: needinfo?(juhsu)
Sure
Flags: needinfo?(juhsu)
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Attachment #8907081 - Flags: feedback?(juhsu)
Attachment #8907083 - Flags: feedback?(juhsu)
Attachment #8907084 - Flags: feedback?(juhsu)
Is you CacheID going to permanent?
It seems that gCacheEntryCurrentId will be 0 for every startup.
Flags: needinfo?(bhsu)
Nope, they won't be permanent.

If I understand Necko correctly, it creates CacheEntries for each HTTP cache access during run-time, and thus I think the ID doesn't have to be persist over restarting Firefox.
Flags: needinfo?(bhsu)
Comment on attachment 8907081 [details] [diff] [review]
P1: Create nsICacheEntry::CacheEntryId

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

lgtm
Attachment #8907081 - Flags: feedback?(juhsu) → feedback+
Comment on attachment 8907083 [details] [diff] [review]
P2: Expose CacheEntryId to nsICacheInfoChannel

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

some minor changes

::: netwerk/base/nsICacheInfoChannel.idl
@@ +48,5 @@
> +   * Note: 0 is reserved for no valid correspnsICacheEntry instances.
> +   * Note: Before calling this method, we should make sure the data is from the
> +   * cache via `isFromCache()`.
> +   */
> +  uint64_t cacheEntryId();

readonly attribute or GetCacheEntryId is more intuitive

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +162,4 @@
>    , NeckoTargetHolder(nullptr)
>    , mSynthesizedStreamLength(0)
>    , mIsFromCache(false)
> +  , mCacheEntryId(0)

Should move this one line below.
Attachment #8907083 - Flags: feedback?(juhsu) → feedback+
Comment on attachment 8907084 [details] [diff] [review]
P3: Create a testcase for nsICacheInfoChannel::cacheEntryId

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

::: netwerk/test/unit/test_cache-entry-id.js
@@ +16,5 @@
> +const responseContent2 = "response body 2";
> +const altContent = "!@#$%^&*()";
> +const altContentType = "text/binary";
> +
> +handlers = [

|var|?

@@ +37,5 @@
> +    handler(metadata, response);
> +    return;
> +  }
> +
> +  response.setStatusLine(metadata.httpVersion, 304, "Not Modified");

If we don't reach here, expose the failure

@@ +100,5 @@
> +  return Promise.resolve()
> +    // Setup testing environment: Placing alternative data into HTTP cache.
> +    .then(_ => fetch(altContentType))
> +    .then(r => check(r, responseContent, "", false,
> +                     cacheEntryId => cacheEntryId === 0))

I realized that empty cache indicates cacheEntryId === 0
Is it by purpose? If yes, mention it in idl.
Attachment #8907084 - Flags: feedback?(juhsu) → feedback+
Attachment #8907083 - Attachment is obsolete: true
Attachment #8907084 - Attachment is obsolete: true
Attachment #8909181 - Flags: feedback+
Attachment #8909182 - Flags: feedback+
Attachment #8907081 - Flags: review?(michal.novotny)
Attachment #8909181 - Flags: review?(michal.novotny)
Attachment #8909182 - Flags: review?(michal.novotny)
Hi Michal,

Do you mind reviewing these patches?

This bug is for providing an entry identifier on nsICacheInfoChannel, which is design for judging whether the data from two different nsICacheInfoChannels is from the same nsICacheEntry. It could be useful when making sure whether the alternative data retrieved from one nsICacheInfoChannel matched the main data from another nsICacheInfoChannel. And the slightly detailed descriptions for each patch are listed below.

Part 1: Create nsICacheEntry::CacheEntryId, which is the unique itself. In order to make it unique, the assignment of the ID is protected by mutex. Aother notable thing about this part is that the identifier of the _OldCacheEntryWrapper is always 0, a reserved valid cache entry.

Part 2: Expose the nsICacheEntry::CacheEntryId to nsHttpChannel and HttpChannelChild. Both of the implementation is guarded by their implementation of nsICacheInfoChannel::IsFromCache().

Part 3: The testcase!
Comment on attachment 8907081 [details] [diff] [review]
P1: Create nsICacheEntry::CacheEntryId

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

::: netwerk/cache2/CacheEntry.cpp
@@ +195,5 @@
> +
> +uint64_t CacheEntry::GetNextId() {
> +  mozilla::MutexAutoLock lock(mLock);
> +  return ++gCacheEntryCurrentId;
> +}

uint64_t CacheEntry::GetNextId() 
{	
  static Atomic<uint64_t, Relaxed> id;
  return ++id;	
}

?

I am not happy with using |this| and the lock from a ctor's initiator.
Attachment #8909182 - Flags: review?(michal.novotny) → review+
Comment on attachment 8907081 [details] [diff] [review]
P1: Create nsICacheEntry::CacheEntryId

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

::: netwerk/cache2/CacheEntry.cpp
@@ +228,4 @@
>  {
>    LOG(("CacheEntry::CacheEntry [this=%p]", this));
>  
> +  MOZ_ASSERT(mCacheEntryId != 0, "ID of a valid CacheEntry shouldn't be 0.");

I know it's almost impossible to overflow uint64_t, but please make sure CacheEntry::GetNextId() cannot return 0.
Attachment #8907081 - Flags: review?(michal.novotny) → feedback+
Attachment #8909181 - Flags: review?(michal.novotny) → review+
It's worth to note that cache API allows to overwrite content of CacheEntry, so this id just ensures that it's the same object not that the data hasn't changed. OTOH when we want to rewrite data we call CacheEntry::Recreate() to create a new CacheEntry.
Attachment #8907081 - Attachment is obsolete: true
Comment on attachment 8913568 [details] [diff] [review]
(V2) P1: Create nsICacheEntry::CacheEntryId

Hi Michal and Honza,

Thanks for the reviews from both of you. Do you mind reviewing this part again?
Attachment #8913568 - Flags: review?(michal.novotny)
Attachment #8913568 - Flags: feedback?(honzab.moz)
(In reply to Michal Novotny (:michal) from comment #24)
> It's worth to note that cache API allows to overwrite content of CacheEntry,
> so this id just ensures that it's the same object not that the data hasn't
> changed. OTOH when we want to rewrite data we call CacheEntry::Recreate() to
> create a new CacheEntry.

It's good to know this. Do you think it's appropriate to place those notes to the idl files?
Comment on attachment 8913568 [details] [diff] [review]
(V2) P1: Create nsICacheEntry::CacheEntryId

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

::: netwerk/cache2/CacheEntry.cpp
@@ +194,5 @@
> +uint64_t CacheEntry::GetNextId() {
> +  static Atomic<uint64_t, Relaxed> id(0);
> +
> +  uint64_t ret = ++id;
> +  MOZ_DIAGNOSTIC_ASSERT(ret != 0, "ID of a valid CacheEntry shouldn't be 0.");

I meant to skip 0 if it overflows, not to move the assertion from CacheEntry::CacheEntry() to CacheEntry::GetNextId().
Attachment #8913568 - Flags: review?(michal.novotny) → feedback+
(In reply to Ben Hsu [:HoPang] from comment #27)
> (In reply to Michal Novotny (:michal) from comment #24)
> > It's worth to note that cache API allows to overwrite content of CacheEntry,
> > so this id just ensures that it's the same object not that the data hasn't
> > changed. OTOH when we want to rewrite data we call CacheEntry::Recreate() to
> > create a new CacheEntry.
> 
> It's good to know this. Do you think it's appropriate to place those notes
> to the idl files?

You can mention it in idl if you want. If you ever experience problem with this, you should consider changing the id so it changes with every data and/or metadata change.
Comment on attachment 8913568 [details] [diff] [review]
(V2) P1: Create nsICacheEntry::CacheEntryId

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

Thanks.

Note: we use 8 lines context for patches.

::: netwerk/cache2/CacheEntry.cpp
@@ +191,4 @@
>                    nsIRunnable,
>                    CacheFileListener)
>  
> +uint64_t CacheEntry::GetNextId() {

nit: { on a new line
Attachment #8913568 - Flags: feedback?(honzab.moz) → feedback+
Hi Michal and sorry for making this modification at this point.

I think reserving zero makes the implementation kind of esoteric if we do want to make the ID generator both thread-safe and skip zero. I am wondering whether it's a good idea to expose the ID generator to `_OldCacheEntryWrapper` and let it get an ID in its ctor as ordinary CacheEntry, since we can make the implementation much simpler. If you see it's acceptable, I'll update the following patches correspondingly.
Attachment #8913568 - Attachment is obsolete: true
Attachment #8915445 - Flags: review?(michal.novotny)
Attachment #8915445 - Flags: feedback+
Comment on attachment 8915445 [details] [diff] [review]
(V3) P1: Create nsICacheEntry::CacheEntryId

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

LGTM, but I don't understand the thread-safety concern and the "skip-zero" issue.
(In reply to Honza Bambas (:mayhemer) from comment #32)
> Comment on attachment 8915445 [details] [diff] [review]
> (V3) P1: Create nsICacheEntry::CacheEntryId
> 
> Review of attachment 8915445 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but I don't understand the thread-safety concern and the "skip-zero"
> issue.

IMHO, if we do want to skip zero, then we might have a code snippet like this.

```
id += (id + 1 == 0) ? 2 : 1;
```

where two different values of 'id' might be acquired from the two `id` accesses, which could make the function return zero in some cases.     

Thus ultimately, we might come up the following code.

```
while (ret = ++id); 
```

which keep finding a non-zero `id` for `ret`, which I personally think this could be a little overkilling for a id generator.
Sorry for writing the wrong code in the previous comment.

IMHO, if we do want to skip zero, then we might have a code snippet like this.

```
id += (id + 1 == 0) ? 2 : 1;
```

where two different values of 'id' might be acquired from the two `id` accesses, which could make the function still return zero in some cases.     

Thus ultimately, we might come up the following code.

```
uint64_t ret = 0;

while (ret == 0) {
  ret = ++id;
} 
```

which keeps finding a non-zero `id` value for `ret`, which I personally think this could be a little overkilling for an id generator.
No.  I don't understand why you so desperately want to skip the zero.  If you start with return ++id, which is 64bit and initially 0, then I don't think you will ever overflow back to zero during a normal use of the browser.
And what if we:

static Atomic<uint64_t, Relaxed> id(1);
return (id += 2);

?

You cut the number of unique ids to half, but you will never hit zero ;)
Attachment #8915445 - Flags: review?(michal.novotny) → review+
(In reply to Ben Hsu [:HoPang] from comment #31)
> I think reserving zero makes the implementation kind of esoteric if we do
> want to make the ID generator both thread-safe and skip zero. I am wondering
> whether it's a good idea to expose the ID generator to
> `_OldCacheEntryWrapper` and let it get an ID in its ctor as ordinary
> CacheEntry, since we can make the implementation much simpler. If you see
> it's acceptable, I'll update the following patches correspondingly.

I don't see any problem with this.
(In reply to Honza Bambas (:mayhemer) from comment #35)
> No.  I don't understand why you so desperately want to skip the zero.  If
> you start with return ++id, which is 64bit and initially 0, then I don't
> think you will ever overflow back to zero during a normal use of the browser.

Because it's silly to not handle the returning value from CacheEntry::GetNextId() while having an assertion that the value must not be zero.
Attachment #8909181 - Attachment is obsolete: true
Attachment #8916931 - Flags: review+
Attachment #8916931 - Flags: feedback+
(In reply to Honza Bambas (:mayhemer) from comment #36)
> And what if we:
> 
> static Atomic<uint64_t, Relaxed> id(1);
> return (id += 2);
> 
> ?
> 
> You cut the number of unique ids to half, but you will never hit zero ;)

In fact, at one moment, I had exactly the same plan in my mind ;P
But I still think making zero legal is a better way to go.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5484914609
Part 1: Create nsICacheEntry::CacheEntryId. r=michal, f=junior,mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e3c67a3872
Part 2: Expose CacheEntryId to nsICacheInfoChannel. r=michal, f=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/7598b4ed02f6
Part 3: Create a testcase for nsICacheInfoChannel::cacheEntryId. r=michal, f=junior
Keywords: checkin-needed
Sorry for the late.

It seems that the part 2 patch needs to be rebased.
Flags: needinfo?(bhsu)
A rebased version
Attachment #8916931 - Attachment is obsolete: true
Attachment #8917754 - Flags: review+
Attachment #8917754 - Flags: feedback+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a984f198478
Part 1: Create nsICacheEntry::CacheEntryId. r=michal, f=junior,mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed4c6ec74ab5
Part 2: Expose CacheEntryId to nsICacheInfoChannel. r=michal, f=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/4896e4d4e3f5
Part 3: Create a testcase for nsICacheInfoChannel::cacheEntryId. r=michal, f=junior
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.