refactor and clean up Service Worker Cache code

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
19 days ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(10 attachments, 15 obsolete attachments)

26.99 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
3.87 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
681 bytes, patch
jgraham
: review+
Details | Diff | Splinter Review
17.11 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
202.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
92.75 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
25.21 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
44.19 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
5.04 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
14.98 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
There are a number of things to refactor and clean up in the Service Worker Cache code.  We don't want to keep the code off on maple any longer, though, as it prevents people from being able to test the current functionality.  Therefore the current idea is to land without this clean up work and fix it before the feature can be enabled by default.

The clean up work includes:

- Remove excessive boiler plate and code duplication in the IPC plumbing.
- Rename IPC types to not have a 'P' prefix.  I've learned thats just for protocols.
- Only export header files needed by external directories.
- Consider removing mozilla::dom::cache namespace for externally exposed classes.
- Smaller refactoring TODOs noted in the code.

I would also put any larger style or structural action items from the review here as well.
Consider replacing the RequestId with a separate actor per request.

Comment 2

4 years ago
(In reply to Ben Kelly [:bkelly] from comment #1)
> Consider replacing the RequestId with a separate actor per request.

Yeah, this was my review comment in https://bugzilla.mozilla.org/show_bug.cgi?id=940273#c31
The "actor per request" pattern is used all over the code.
See bug 940273 comment 207 for additional style and refactor suggestions.
(Assignee)

Updated

4 years ago
Blocks: 1143504
(Assignee)

Updated

4 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I'm going to build this on top of the patches in bug 1110814.
Depends on: 1110814
(Assignee)

Updated

4 years ago
Depends on: 1120501
Work-in-progress of the refactor.  This compiles, but does not pass tests yet.

Before patch:    740K    dom/cache/
After patch:     716K    dom/cache/

A net reduction of about 24KB of boilerplate code.
(Assignee)

Updated

4 years ago
Depends on: 1150608
(Assignee)

Updated

4 years ago
Depends on: 1150691
Andrea, this patch is a bit ugly because a lot of code got replaced.  Let me try to highlight the important bits:

1) Look at the .ipdl changes.  The PCacheOp protocol is essentially replaces all of the methods on PCache and PCacheStorage.
2) The type of operation and their results are defined in PCacheTypes.ipdl as unions.
3) CacheOpChild and CacheOpParent are the actor implementations.  These are created for each operation requested by content.
4) The union for the op is pushed all the way into Manager which has just a few entry points now.  ExecuteCacheOp(), ExecuteStorageOp(), and ExecutePutAll().
5) Because of the way everything is sent in either CacheOpArgs or CacheOpResult, the AutoUtils.cpp code basically just has one auto type for each of these unions now.

As I mentioned in a previous comment this patch removes a ton of boilerplate and nets a reduction of about 24KB worth of code.
Attachment #8587108 - Attachment is obsolete: true
Attachment #8587661 - Flags: review?(amarchesini)
Interdiff for the first patch to fix a few references to a removed header I only noticed after a clobber build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=139336b7cb7f
(Assignee)

Updated

4 years ago
Attachment #8587661 - Attachment description: Refactor Cache IPC requests to use a separate actor. r=baku → P1 Refactor Cache IPC requests to use a separate actor. r=baku
Comment on attachment 8587661 [details] [diff] [review]
P1 Refactor Cache IPC requests to use a separate actor. r=baku

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

::: dom/cache/CacheChild.cpp
@@ +72,5 @@
>    if (!listener) {
>      return;
>    }
>  
> +  // TODO: only destroy if there are no ops or push streams still running

Note, I intend to do this in a later patch in this bug.  I figured I should start splitting things into separate patches as this was getting quite big.
This is a mostly mechanical patch to remove the P prefix from the Cache IPC types.  For example, PCacheRequest becomes CacheRequest, etc.  I think I also removed some stale header includes I noticed while doing this.
Attachment #8587771 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Blocks: 1120501
No longer depends on: 1120501
Another mechanical patch to fix non-protocol IPC type names.  This one switches the fetch PHeadersEntry to just HeadersEntry.  Also InternalHeaders::GetPHeaders() changes to InternalHeaders::GetIPCHeaders().
Attachment #8587777 - Flags: review?(nsm.nikhil)
Comment on attachment 8587777 [details] [diff] [review]
P3 Modify Fetch IPC HeaderEntry not to use a 'P' prefix in its type name. r=nsm

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

::: dom/fetch/InternalHeaders.h
@@ +91,5 @@
>    static already_AddRefed<InternalHeaders>
>    CORSHeaders(InternalHeaders* aHeaders);
>  
>    void
> +  GetIPCHeaders(nsTArray<HeadersEntry>& aHeadersOut) const;

Or would you prefer that we just move the IPC type into Cache?  No one else is using it right now.
Comment on attachment 8587777 [details] [diff] [review]
P3 Modify Fetch IPC HeaderEntry not to use a 'P' prefix in its type name. r=nsm

After thinking about it, I just want to move the type.  I'll reflag later today.
Attachment #8587777 - Flags: review?(nsm.nikhil)
This moves all the IPC gunk out of Fetch and into Cache.  It also does the type rename as per the original patch.
Attachment #8587777 - Attachment is obsolete: true
Attachment #8588038 - Flags: review?(nsm.nikhil)
This patch completes the actor lifetime changes I made in P1.  It keeps the actors alive during async operations even when the worker has Notify()'d its WorkerFeatures.  This is something Ben Turner recommended in previous review.

Note, I only hold the worker open for a PCacheStreamControlChild actor if the streams are actively being read.  If the stream has never been read, then we go ahead and close.  This is to handle the case where some gets a Response from the Cache, but only looks at headers and not the body.
Attachment #8588185 - Flags: review?(amarchesini)
Comment on attachment 8588038 [details] [diff] [review]
P3 Move Fetch IPC PHeaderEntry type to Cache. Rename HeadesEntry. r=nsm

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

::: dom/cache/TypeUtils.cpp
@@ +135,5 @@
> +ToHeadersEntryList(nsTArray<HeadersEntry>& aOut, InternalHeaders* aHeaders)
> +{
> +  MOZ_ASSERT(aHeaders);
> +
> +  nsAutoTArray<InternalHeaders::Entry, 64> entryList;

64 seems really big for headers. How about 16? My completely unscientific glossing of a few entries here https://github.com/http2/http_samples seems to suggest ~10 headers usually.

@@ +423,5 @@
> +
> +  for (uint32_t i = 0; i < aHeadersEntryList.Length(); ++i) {
> +    const HeadersEntry& headersEntry = aHeadersEntryList[i];
> +    entryList.AppendElement(InternalHeaders::Entry(headersEntry.name(),
> +                                                   headersEntry.value()));

Could you use the non-copying form?
InternalHeaders::Entry* entry = entryList.AppendElement();
entry->mName = headersEntry.name();
entry->mValue = headersEntry.value();
Attachment #8588038 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8587661 [details] [diff] [review]
P1 Refactor Cache IPC requests to use a separate actor. r=baku

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

Thanks for doing it. The code is much more readable and easy to follow now!

::: dom/cache/AutoUtils.cpp
@@ +325,5 @@
> +
> +      break;
> +    }
> +    default:
> +      MOZ_CRASH("Cache args type cannot send a Requesst/Response pair!");

Request

@@ +452,5 @@
> +      break;
> +    }
> +    default:
> +    {
> +      MOZ_CRASH("Cache result type cannot handle returning a Response!");

you don't have {} in the other default: MOZ_CRASH...

@@ +482,5 @@
> +      break;
> +    }
> +    default:
> +    {
> +      MOZ_CRASH("Cache result type cannot handle returning a Request!");

here too.

::: dom/cache/Cache.cpp
@@ +104,2 @@
>    nsRefPtr<InternalRequest> ir = ToInternalRequest(aRequest, IgnoreBody, aRv);
>    if (aRv.Failed()) {

NS_WARN_IF here too?

@@ +113,3 @@
>  
> +  args.Add(ir, IgnoreBody, PassThroughReferrer, IgnoreInvalidScheme, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/cache/CacheOpParent.cpp
@@ +62,5 @@
> +
> +  Execute(manager);
> +}
> +
> +

2 lines. Usually you have 1 single line between methods.

::: dom/cache/CacheStorage.cpp
@@ +52,5 @@
>  NS_INTERFACE_MAP_END
>  
> +// We cannot reference IPC types in a webidl binding implementation header.  So
> +// define this in the .cpp and use heap storage in the mPendingRequests list.
> +struct CacheStorage::Entry

final

@@ +184,5 @@
> +  }
> +
> +  nsRefPtr<InternalRequest> request = ToInternalRequest(aRequest, IgnoreBody,
> +                                                        aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/cache/CacheStorageParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/dom/cache/CacheStorageParent.h"
>  
> +#include "mozilla/unused.h"

should this goes after line 14?

::: dom/cache/Manager.h
@@ +119,3 @@
>  
>    protected:
>      ~Listener() { }

virtual ?

::: dom/cache/PCache.ipdl
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  include protocol PBackground;
>  include protocol PCachePushStream;
> +include protocol PCacheOp;

alphabetic order.
Attachment #8587661 - Flags: review?(amarchesini) → review+
Attachment #8587771 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #16)
> ::: dom/cache/CacheStorageParent.cpp
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "mozilla/dom/cache/CacheStorageParent.h"
> >  
> > +#include "mozilla/unused.h"
> 
> should this goes after line 14?

I like to sort on dir before file.  So I put all the files directly in mozilla/ first.

> ::: dom/cache/Manager.h
> @@ +119,3 @@
> >  
> >    protected:
> >      ~Listener() { }
> 
> virtual ?

No, because this is never deleted through a base pointer.  This is only declared so that its protected to help ensure that invariant.
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #15)
> @@ +423,5 @@
> > +
> > +  for (uint32_t i = 0; i < aHeadersEntryList.Length(); ++i) {
> > +    const HeadersEntry& headersEntry = aHeadersEntryList[i];
> > +    entryList.AppendElement(InternalHeaders::Entry(headersEntry.name(),
> > +                                                   headersEntry.value()));
> 
> Could you use the non-copying form?
> InternalHeaders::Entry* entry = entryList.AppendElement();
> entry->mName = headersEntry.name();
> entry->mValue = headersEntry.value();

It seems nsTArray::AppendElement() takes T&&.  An immediate T() is of type T&&, so the compiler can just optimize the temporary away with a move.

I think you only need the other form when you are appending a long lived object that can't be moved.
Attachment #8587661 - Attachment is obsolete: true
Attachment #8587764 - Attachment is obsolete: true
Attachment #8591816 - Attachment is obsolete: true
Attachment #8591832 - Flags: review+
This removes the useless DBSchema type.  I still use a separate "db" namespace, though, to avoid conflicting on symbols like CacheMatch.

Note, I purposely left the methods in the .cpp in the existing order to make the patch easier to read.  I may do a follow-up to reorder them so that the declarations aren't needed in the .cpp, but I don't think that's necessary right now.
Attachment #8591879 - Flags: review?(ehsan)
Similar patch, but for FileUtils.
Attachment #8591886 - Flags: review?(ehsan)
Fix an old TODO to clarify the difference between DeleteCache() and CacheDelete() in DBSchema.h.  I chose rename things so that we have:

  CreateCacheId()
  DeleteCacheId()

To distinguish functions that clearly operate on the life cycle of a Cache ID.
Attachment #8591893 - Flags: review?(ehsan)
These WPT tests are new as of this weekend... Also I would note this was a case of a test passing.

I'll look at it tomorrow.
Flags: needinfo?(bkelly)

Updated

4 years ago
Attachment #8591879 - Flags: review?(ehsan) → review+

Comment 30

4 years ago
Comment on attachment 8591886 [details] [diff] [review]
P6 Replace useless cache::FileUtils type with cache::file namespace. r=ehsan

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

Do we really gain anything by adding a new nested namespace for these functions?  As far as I can tell, there are no name clashes if you just put all of these in mozilla::dom::cache, and that way it would be a bit less painful to work with this code (fewer guesses as to which namespace you need to type out in the code/debugger, etc.)  But r=me either way, I can also later change this to remove this namespace if you don't want to do it right now.
Attachment #8591886 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8591893 - Flags: review?(ehsan) → review+
The refactor fixed a bug in CacheStorageParent::RecvKeys() where it accidentally queued an OP_DELETE instead of OP_KEYS when verification was still happening.

The refactor fixes this.  So the test should expect to pass now.
Attachment #8592254 - Flags: review?(james)
Comment on attachment 8588185 [details] [diff] [review]
P4 Keep Cache Actors alive during async operations. r=baku

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

lgtm!

::: dom/cache/ReadStream.cpp
@@ +287,5 @@
>  {
>    // stream ops can happen on any thread
>    MOZ_ASSERT(aNumReadOut);
>  
> +  if (aCount) {

can it really happen that we have aCount == 0 ?

@@ +308,5 @@
>  {
>    // stream ops can happen on any thread
>    MOZ_ASSERT(aNumReadOut);
>  
> +  if (aCount) {

same here.
Attachment #8588185 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #32)
> can it really happen that we have aCount == 0 ?

Hmm, I thought it could happen here via NS_InputStreamIsBuffered(), but I guess that passes 1.  I'll re-work that bit:

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#714
Attachment #8592254 - Flags: review?(james) → review+
Updated to remove the file:: namespace.
Attachment #8591886 - Attachment is obsolete: true
Attachment #8592288 - Flags: review+
Updated to check bytes read in ReadSegments() instead of aCount before setting "ever read" flag.
Attachment #8588185 - Attachment is obsolete: true
Attachment #8592290 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 1154531
(Assignee)

Updated

4 years ago
Blocks: 1151892
sorry had to back this out for causing frequent windows xp bc1 memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=8868557&repo=mozilla-inbound
I'll look, but as far as I know this code doesn't run in bc1 at all.
Flags: needinfo?(bkelly)
I need to rebase on top of bug 1149987 where the IPC nsresult for one operation was converted to an ErrorResult. Since my refactor unifies error reporting I now need to convert all my nsresult values into an ErrorResult.

This patch makes that a bit more convenient by letting me do:

  // in header
  void OnOpComplete(ErrorResult&& aRv);

  // code using OnOpComplete
  OnOpComplete(ErrorResult(NS_ERROR_FAILURE));

I also de-duped some code in ErrorResult.h here.  I believe this should be inlined by the compiler and have no performance impact.
Attachment #8593142 - Flags: review?(bzbarsky)
Bad hg fold in the last patch.  This one has the missing hunks to init values in the new constructor.
Attachment #8593142 - Attachment is obsolete: true
Attachment #8593142 - Flags: review?(bzbarsky)
Attachment #8593143 - Flags: review?(bzbarsky)
Painful refactor as I needed to propagate the ErrorResult type far enough out for the FetchPut to pass return its TypeError.
Attachment #8591832 - Attachment is obsolete: true
Attachment #8593146 - Flags: review+
Andrea, this patch is much smaller than I thought when we talked on IRC.  If you could prioritize it, this is blocking bug 1151892 which people keep asking for.

Side note, I originally wanted to make a error type in the union to make it clear you either get an error or a real result.  Unfortunately, the ErrorResult cannot be placed in an IPDL union because it doesn't support copy construction or assignment.  So instead I just enforce that the result is void_t() when an error is passed.

Thanks!
Attachment #8593165 - Flags: review?(amarchesini)
Comment on attachment 8593143 [details] [diff] [review]
P0 Add an ErrorResult constructor that takes nsresult. r=bz

The new ctor does not init mResult before calling AssignErrorCode(), but AssignErrorCode asserts various stuff about mResult not having certain values.  This might happen to work by accident, but it's clearly not good.

I think you either need to set mResult = NS_OK in the new ctor before calling AssignErrorCode, or move mResult = NS_OK into Init(), or go ahead and just have the new ctor delegate to the no-arg one, so you don't need Init at all.  And then you can either do *this = aRv, or keep the AssignErrorCode thing, which I admit does reduce the code duplication between Throw() and operator=.

Also, I would prefer that all the private methods you're adding go above the private members.  I know the IPC friend decl and methods are already misplaced; please move them there too.

r=me with those fixed
Attachment #8593143 - Flags: review?(bzbarsky) → review+
Comment on attachment 8593165 [details] [diff] [review]
P8 Correctly set the Feature on the stream control child actor. r=baku

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

lgtm!

::: dom/cache/CacheOpChild.cpp
@@ +20,5 @@
> +namespace {
> +
> +void
> +AddFeatureToStreamChild(const CacheReadStream& aReadStream, Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);

@@ +30,5 @@
> +}
> +
> +void
> +AddFeatureToStreamChild(const CacheResponse& aResponse,
> +                        Feature* aFeature)

this can stay on the same line. probably..

@@ +31,5 @@
> +
> +void
> +AddFeatureToStreamChild(const CacheResponse& aResponse,
> +                        Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);

@@ +41,5 @@
> +}
> +
> +void
> +AddFeatureToStreamChild(const CacheRequest& aRequest,
> +                        Feature* aFeature)

this can stay on the same line. probably..

@@ +42,5 @@
> +
> +void
> +AddFeatureToStreamChild(const CacheRequest& aRequest,
> +                        Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);
Attachment #8593165 - Flags: review?(amarchesini) → review+
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.