Closed Bug 1290951 Opened 4 years ago Closed 3 years ago

extend byte-for-byte update check to all service worker importScripts() resources

Categories

(Core :: DOM: Service Workers, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bkelly, Assigned: bhsu)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 45 obsolete files)

17.38 KB, text/plain
Details
9.65 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
14.85 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
21.27 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
2.12 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
13.28 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
11.53 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
3.66 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
7.42 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
5.91 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
1.62 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
See:

https://github.com/slightlyoff/ServiceWorker/issues/839#issuecomment-236256162

Basically this means iterating over every entry in the service worker's associated CacheStorage and performing a byte-for-byte check.  This can be done in parallel.
Hi Ben, I guess this is something between P2 (fix in a few months) and P3 (backlog). I am setting P3 for now. Please change it as you see fit. Thanks!
Priority: -- → P3
Assignee: nobody → bhsu
As [0] suggest, we should take care double-download issue in this bug.

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1290944#c55
CompareManager. r=bkelly
Attachment #8840311 - Attachment is obsolete: true
Attachment #8840377 - Attachment is obsolete: true
Hello Ben,

To my best knowledge, most of the work needed for extending byte-check to imported scripts has finished with those patches. However, I've come across a problem caused by an error handling of performing byte-check for important scripts. Say there is a main script(A) which requires an imported script(B), after an update, there comes a new (A) which doesn't need (B) anymore, and thus (B) is no longer available on the web. To make the registration still updatedable , I loose the check of the result of fetching new scripts in Patch 3.2 (not covered by the provided testcase). However, I have no Idea whether I should do this. If yes, I don't know how I can write a wpt testcase for that, since I cannot think of a way to make a wpt python server returns two different content with the very same URL, where one requires an imported script, while the other doesn't.

And I am sorry for asking you this question at this moment, I have no intention of pushing you to help me or reviewing these patches.
(In reply to Ben Hsu [:HoPang] from comment #16)
> scripts. Say there is a main script(A) which requires an imported script(B),
> after an update, there comes a new (A) which doesn't need (B) anymore, and
> thus (B) is no longer available on the web. To make the registration still
> updatedable , I loose the check of the result of fetching new scripts in
> Patch 3.2 (not covered by the provided testcase). However, I have no Idea
> whether I should do this. If yes, I don't know how I can write a wpt
> testcase for that, since I cannot think of a way to make a wpt python server
> returns two different content with the very same URL, where one requires an
> imported script, while the other doesn't.

I think this is reasonable.

Just to make sure I understand, though, what happens if the importScripts() failure is temporary?  As in, there actually isn't an update.  I assume we don't remove currently installed set of scripts since the new install will fail.  Is that right?

> And I am sorry for asking you this question at this moment, I have no
> intention of pushing you to help me or reviewing these patches.

No problem.  I'd like to see this land, so I wouldn't mind reviewing even though my queue is closed.  If its ready for review, please NI me.  Thanks!
Status: NEW → ASSIGNED
> Just to make sure I understand, though, what happens if the importScripts()
> failure is temporary?  As in, there actually isn't an update.  I assume we
> don't remove currently installed set of scripts since the new install will
> fail.  Is that right?

It's an interesting question, since every service-worker has its own cache saving its scripts, we definitely keep those scripts intact. However, when there isn't an update and the network failure is temporary, with those patches applied, we'll try to create a new service-worker, which might override the existing installing, waiting or even the active service worker eventually if all the `importScripts()` succeed...

After thinking a little deeper, I think we should assume those two versions of scripts are the same in case the network failure is temporary. Say, it the missing is deliberately, then at least one other script must be different, and then we can still know there should be a new service worker. OTOH, if the missing is temporary, then we don't create a new service-worker for it. Nonetheless, there still is a pitfall caused by assuming there is no update when a network failure occurs. That is, there is actually an update, and the update is the missing script. However, merely given by a missing scripts, we can never know whether there is actually update, so I prefer assuming a missing script stands for no-update. Does this make any sense to you?
Yea, I think you are right we should fail the comparison if the importScripts() fails.  If its deliberate, then they must remove the importScripts() in the parent script which will trigger the update.  If it keeps an importScripts() to a dead URL then the install will fail.
Attachment #8840382 - Attachment is obsolete: true
H Ben, 

After writing a  testcase for the case we've discussed before, I think those patches are ready for review now. There are four groups of patches, and their purposes are listed as follows.

Patch 1.X: Merely get the URL lists before doing byte-check.
Patch 2.X: Refactor some minor stuff,
           Change the relationship between CompareManager, CompareNetwork and CcompareCache.
Patch 3.X: Extend the bytecheck to imported scripts.
Patch 4.X: Add a wpt testcase for general cases
           Add a mochitest for an edge case.

Since there are functions with the same name in CompareManager, CompareNetwork and CcompareCache, I moved their implementations out of their class definitions to make the file easier to maintain. The reason why I choose mochitest for patch 4.2 is that I have to get different responses with an identical URL, so I need a stateful server.
Attachment #8840371 - Flags: review?(amarchesini)
Attachment #8840372 - Flags: review?(amarchesini)
Attachment #8840373 - Flags: review?(amarchesini)
Attachment #8840374 - Flags: review?(amarchesini)
Attachment #8840376 - Flags: review?(amarchesini)
Attachment #8840378 - Flags: review?(amarchesini)
Attachment #8840379 - Flags: review?(amarchesini)
Attachment #8840380 - Flags: review?(amarchesini)
Attachment #8840381 - Flags: review?(amarchesini)
Attachment #8840384 - Flags: review?(amarchesini)
Attachment #8841928 - Flags: review?(amarchesini)
Attachment #8841929 - Flags: review?(amarchesini)
Hi Andrea, 

This bug is to extend byte-check to imported scripts. With those patches applied, we fetch the new versions of the scripts of a service worker, compare them with the existing scripts and finally install the service worker if needed.

Since Ben Kelly is recently too busy to review this bug, he suggested me asking for your review. Comment 22 is the high level overview of those patches. In addition, I'd like to explain how the relationship of CompareManager, CompareNetwork and CompareCache is changed in Patch 2.X and Patch 3.X.

[Current Design]
CompareManager (1) --- (1) CompareNetwork
                   \-- (1) CompareCache

[Patch 2.X]
ComapreManager (1) --- (1) CompareNetwork (1) --- (1) CompareCache

[Patch 3.X]
ComapreManager (1) --- (N) CompareNetwork (1) --- (1) CompareCache
Note: a ComapreNetwork is created for one source script of the service worker to compared.

The other thing is that, after the review process, I'll update the reviewer in the commit messages :)
Attachment #8840371 - Flags: review?(amarchesini) → review+
Hi Ben, sorry for the delay. I'm currently working on a couple of multi-e10s blocker bugs.
I'll be back reviewing your code next week. I hope it's OK for you.
Hi Andrea,

It's totally fine. I am now working on a prototype for 1246289 and 1267119 which I originally assumed blocked by this bug, hoping the prototype will work :)
I'm going to try to review this since I'm sitting at the same table with Ho-Pang this week.
Flags: needinfo?(bkelly)
Attachment #8840372 - Flags: review?(amarchesini)
Attachment #8840373 - Flags: review?(amarchesini)
Attachment #8840374 - Flags: review?(amarchesini)
Attachment #8840376 - Flags: review?(amarchesini)
Attachment #8840378 - Flags: review?(amarchesini)
Attachment #8840379 - Flags: review?(amarchesini)
Attachment #8840380 - Flags: review?(amarchesini)
Attachment #8840381 - Flags: review?(amarchesini)
Attachment #8840384 - Flags: review?(amarchesini)
Attachment #8841928 - Flags: review?(amarchesini)
Attachment #8841929 - Flags: review?(amarchesini)
Comment on attachment 8840372 [details] [diff] [review]
Part 1.2: Get the script URL list before actual comparing

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +403,4 @@
>    Cleanup();
>  
>    void
> +  FetchScript() {

nit: curly brace on following line

MOZ_ASSERT(mState == WaitingForScriptOrComparisonResult)

@@ +467,5 @@
> +      Fail(NS_ERROR_FAILURE);
> +      return;
> +    }
> +
> +    uint32_t len;

nit: initialize to 0 for sanity
Attachment #8840372 - Flags: review+
Attachment #8840373 - Flags: review+
Attachment #8840374 - Flags: review+
Comment on attachment 8840376 [details] [diff] [review]
Part 2.3: Update the lifecycle of CompareCache

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +142,4 @@
>  
>    nsresult
>    Initialize(nsIPrincipal* aPrincipal, const nsAString& aURL,
> +             Cache* const aCache);

I don't see `* const` much.  I guess its ok, but unusual.

@@ +882,5 @@
>    return NS_OK;
>  }
>  
>  void
> +CompareCache::Finished(nsresult aStatus, bool aInCache)

Can you maybe add a follow-on patch to change the bare boolean to an enumeration.  It would be a lot easier to read Finish(rv, InCache) or Finish(rv, NotInCache) instead of just true/false.

@@ +901,5 @@
>  
> +    if (mPump) {
> +      mPump->Cancel(NS_BINDING_ABORTED);
> +      mPump = nullptr;
> +    }

Should Abort() call mManager->CacheFinished(NS_BINDING_ABORTED)?  It seems weird that we always call the manager when we hit an error or success completion, but not when we are aborted.
Attachment #8840376 - Flags: review+
Comment on attachment 8840378 [details] [diff] [review]
Part 2.4: Add mIsMainScript to CompareNetwork

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +100,5 @@
>  
>    nsresult
> +  Initialize(nsIPrincipal* aPrincipal,
> +             const nsAString& aURL,
> +             bool aIsMainScript,

Using an enum might be a bit more readable.  MainScript vs ImportedScript, etc.  Can be a follow-on bug.
Attachment #8840378 - Flags: review+
Comment on attachment 8840379 [details] [diff] [review]
Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +114,5 @@
>      return mBuffer;
>    }
>  
> +  const nsString&
> +  URL() const

This isn't used in this patch.  Is it used in a later patch?

@@ +134,5 @@
> +    return internalHeaders.forget();
> +  }
> +
> +  UniquePtr<mozilla::ipc::PrincipalInfo>
> +  MovePrincipalInfo()

nit: More typical naming in gecko would be TakePrincipalInfo().

@@ +754,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  UniquePtr<mozilla::ipc::PrincipalInfo> principalInfo(new mozilla::ipc::PrincipalInfo());

Maybe add to the top of the file:

  using mozilla::ipc::PrincipalInfo;

Also, I think its slightly prefered to use MakeUnique<PrincipalInfo>() here.

@@ +756,5 @@
> +  }
> +
> +  UniquePtr<mozilla::ipc::PrincipalInfo> principalInfo(new mozilla::ipc::PrincipalInfo());
> +  rv = PrincipalToPrincipalInfo(channelPrincipal, principalInfo.get());
> +  

nit: trailing white space
Attachment #8840379 - Flags: review+
Comment on attachment 8840380 [details] [diff] [review]
Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache

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

I think this is ok, but the complexity of the ScriptLoader makes it hard to review.  At this point I think we are going to need good tests.
Attachment #8840380 - Flags: review+
Comment on attachment 8840381 [details] [diff] [review]
Part 3.1: Extend the bytecheck to imported scripts

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

I think we need to replace the `mURL == URL` code for detecting main script here.  It seems we should be able to track the top level script explicitly?

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +342,5 @@
>        return;
>      }
>  
> +    mAreScriptsEqual = mAreScriptsEqual && aIsEqual;
> +    if (--mPendingCount) {

MOZ_DIAGNOSTIC_ASSERT(mPendingCount > 0) before this decrement.

@@ +385,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        Fail(rv);
>      }
> +
> +    mCNs.AppendElement(cn);

nit: You can do mCNs.AppendElement(cn.forget()) to save a set of AddRef()/Release() calls here.

@@ +467,4 @@
>  
>        nsString URL;
>        request->GetUrl(URL);
> +      FetchScript(URL, mURL == URL /* aIsMainScript */, mOldCache);

I would feel a little bit better if we explicitly tracked the main script instead of relying on the URL being different.  It seems possible you could write a script that importScripts() itself again, but only a limited number of times.

Can you add a follow-on patch to do this?

@@ +1220,4 @@
>    }
>  
>    MOZ_ASSERT(mState == WaitingForPut);
> +  if (--mPendingCount) {

Again, please do a MOZ_DIAGNOSTIC_ASSERT() when decrementing to ensure we don't underflow.
Attachment #8840381 - Flags: review+
Attachment #8840384 - Flags: review+
Comment on attachment 8841928 [details] [diff] [review]
Part 3.2: Tolerate missing imported scripts

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +731,5 @@
>    if (NS_FAILED(mNetworkResult)) {
> +    // An imported script could become offline, since it might no longer be
> +    // needed by the new importing script. In that case, the importing script
> +    // must be different, and thus, it's okay to report same script found here.
> +    rv = mIsMainScript ? mNetworkResult : NS_OK;

Can we add a WPT test for this condition?  I think if you store a cookie on the importScripts() url you could fake a network failure from the server .py the second time you see the load.
Attachment #8841928 - Flags: review+
Comment on attachment 8841929 [details] [diff] [review]
Part 4.2: Add a mochitest for an useless imported script after an update

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

Can you just rename this test to be human readable?  Maybe something like test_sw_update_offline_import.html?

::: dom/workers/test/serviceworkers/bug1290951_worker_imported.sjs
@@ +16,5 @@
> +    setState("count", "2");
> +  }
> +  // For all subsequent requests, return the second source.
> +  else {
> +    response.setStatusLine(request.httpVersion, 404, "Not found");

Is it not possible to do this sort of thing in a WPT test?
Attachment #8841929 - Flags: review+
Flags: needinfo?(bkelly)
Comment on attachment 8840376 [details] [diff] [review]
Part 2.3: Update the lifecycle of CompareCache

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +901,5 @@
>  
> +    if (mPump) {
> +      mPump->Cancel(NS_BINDING_ABORTED);
> +      mPump = nullptr;
> +    }

I do this deliberately, since after this refactor, abort() should always be triggered from mManager, there is no need to go to the mManager again.
Comment on attachment 8841929 [details] [diff] [review]
Part 4.2: Add a mochitest for an useless imported script after an update

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

::: dom/workers/test/serviceworkers/bug1290951_worker_imported.sjs
@@ +16,5 @@
> +    setState("count", "2");
> +  }
> +  // For all subsequent requests, return the second source.
> +  else {
> +    response.setStatusLine(request.httpVersion, 404, "Not found");

I can try rewriting this with cookie., but I'd like to land these patches in case of more rebasing work, could I leave the test this way in this bug and create a new bug for rewriting the test?
Comment on attachment 8840381 [details] [diff] [review]
Part 3.1: Extend the bytecheck to imported scripts

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +467,4 @@
>  
>        nsString URL;
>        request->GetUrl(URL);
> +      FetchScript(URL, mURL == URL /* aIsMainScript */, mOldCache);

Hmm, it's a very interesting question. After thinking over it, I don't think we have to explicitly track the main script, since scripts saved in the cache are merely for comparison, which have nothing to do with evaluation. On the other hand, for a script being imported for more than one time in the previous service worker, there should be only one copy of the script in the cache, because the following imports uses the very same cached one. Does it make sense to you?
Update the reviewer id.
Attachment #8855651 - Attachment description: Part 1.1: Move some function implementations out of CompareManager → (V2) Part 1.1: Move some function implementations out of CompareManager
Attachment #8855651 - Flags: review+
Attachment #8840371 - Attachment is obsolete: true
Attachment #8840372 - Attachment is obsolete: true
Attachment #8840379 - Attachment is obsolete: true
Attachment #8840381 - Attachment is obsolete: true
Attachment #8855653 - Flags: review+
Attachment #8855654 - Flags: review+
Attachment #8855655 - Flags: review+
Attachment #8840376 - Attachment is obsolete: true
Attachment #8855656 - Attachment description: Part 2.3: Update the lifecycle of CompareCache → (V2) Part 2.3: Update the lifecycle of CompareCache
Attachment #8855656 - Flags: review+
Attachment #8840378 - Attachment is obsolete: true
Attachment #8840380 - Attachment is obsolete: true
Attachment #8841929 - Attachment is obsolete: true
Attachment #8840384 - Attachment is obsolete: true
Hmm, the try result doesn't seem well. There is memory leaked...
Fix the memory leak mentioned in comment 47.
Fix the memory leak mentioned in comment 47.
Attachment #8855654 - Attachment is obsolete: true
Attachment #8855658 - Attachment is obsolete: true
Attachment #8855655 - Attachment is obsolete: true
Attachment #8857792 - Flags: review+
Attachment #8857788 - Flags: review+
Attachment #8857787 - Flags: review+
Attachment #8855662 - Flags: review+
Attachment #8855661 - Flags: review+
Attachment #8855657 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d518009bfe70
Part 1.1: Move some function implementations out of CompareManager. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a459d48f1d86
Part 1.2: Get the script URL list before actual comparing. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f30a5669251
Part 2.1: Move some functions out of their class definitions. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/437aa650b9d9
Part 2.2: Make CompareManager::ComparisonFinished() public. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/779deac689f9
Part 2.3: Update the lifecycle of CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1c5c05eb58
Part 2.4: Add mIsMainScript to CompareNetwork. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a8e0001d78
Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e133f3b768c
Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b47c4e9e3b
Part 3.1: Extend the bytecheck to imported scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be709822325
Part 3.2: Tolerate missing imported scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2793d9e7c1d2
Part 4.1: Add a new wpt test for extended bytecheck. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/c351046112bf
Part 4.2: Add a mochitest for an useless imported script after an update. r=bkelly
Keywords: checkin-needed
Blocks: 1357290
backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e for causing Bug 1357437
Status: RESOLVED → REOPENED
Flags: needinfo?(bhsu)
Resolution: FIXED → ---
Thanks, I'll check them again.
Flags: needinfo?(bhsu)
Attachment #8864253 - Attachment description: Part 3.1: Extend the bytecheck to imported scripts → (V4) Part 3.1: Extend the bytecheck to imported scripts
Attachment #8864254 - Attachment description: Part 3.2: Tolerate missing imported scripts → (V2) Part 3.2: Tolerate missing imported scripts
Attachment #8857792 - Attachment is obsolete: true
Attachment #8841928 - Attachment is obsolete: true
Attachment #8855661 - Attachment is obsolete: true
Comment on attachment 8864258 [details] [diff] [review]
(V3) Part 4.1: Add a new wpt test for extended bytecheck

Rebased
Attachment #8864258 - Flags: review+
Hi Ben, and sorry for the late.

I think those patches are ready :)
Do you mind reviewing them?

Firstly, I'd like to explain how 1357437 crashes. The story goes like this, one of the imported script is failed to load in a very early stage (NS_NewChannel), and later the handling CompareManager responses to the CompareCallback.When loading the main script, we want to save the LoadFlag to the CompareCallback, however, the CompareManager release the CompareCallback due to the previous error.

In Patch 2.7, to fix the incorrect error handling, I simplified the relationship between ComapreCallback, CompareManager, CompareNetwork and CompareCache, and I ensure CompareManager can only notify CompareCallback once via checking the state of CompareManager. In addition, I merge the the constructors of CompareManager, CopmareNetwork and CompareCache with their Initialize(), since I want to make the error handling code can be shared among initialize() and other operations.

In Patch 2.8, as its description suggests, I removed CompareManager::SetMaxScope() and CompareManager::SaveLoadFlags() to simply the code flow. Now both MaxScope and LoadFlags are set to CompareManager only after the CompareNetwork is finished, and they are passed to the CompareCallback by the CompareManager. Since CompareManagers is guarded by state, we can make sure there won't be using-after-free when the setting them to CompareCallbacks.

Patch 3.1 and Patch 3.2 are just rebased versions.
Flags: needinfo?(bkelly)
There is also one thing to apologize for. I should use enums instead of bools, I think I'll file a new bug for that after the landing of those patches, since this bug is blocking other bugs and I think the new bug will be a great good first bug.
Comment on attachment 8864251 [details] [diff] [review]
Part 2.7: Unify how CompareManager, CompareNetwork and CompareCahce report errors during initialization and other operations

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

Looks good to me, but I think we should convert to ScopeExit() reduce the chance we forget to call the right completion method.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +410,4 @@
>      MOZ_ASSERT(mState == WaitingForExistingOpen);
>  
>      if (NS_WARN_IF(!aValue.isObject())) {
> +      NotifyComparisonResult(NS_ERROR_FAILURE);

Can you add a follow-on patch to convert this to ScopeExit?  See:

https://dxr.mozilla.org/mozilla-central/source/mfbt/ScopeExit.h

Its an RAII helper for ensuring a particular method is called when you exit the scope.  So you can do something like:

  nsresult rv = NS_FAILURE;
  auto guard = MakeScopeExit([&] {
    NotifyComparisonResult(rv);
  });

  if (NS_WARN_IF(!aValue.isObject())) {
    // scope exit calls the notify comparison
    return;
  }

  // success, don't notify here
  guard.release()
  return;

I think something like this would make this code a lot less error prone.
Attachment #8864251 - Flags: review+
Attachment #8864252 - Flags: review+
Flags: needinfo?(bkelly)
Comment on attachment 8864253 [details] [diff] [review]
(V4) Part 3.1: Extend the bytecheck to imported scripts

Rebased version.
Attachment #8864253 - Flags: review+
Comment on attachment 8864254 [details] [diff] [review]
(V2) Part 3.2: Tolerate missing imported scripts

Rebased version.
Attachment #8864254 - Flags: review+
Rebased - Fastforward
Attachment #8855656 - Attachment is obsolete: true
Attachment #8857788 - Attachment is obsolete: true
Attachment #8865749 - Attachment description: Part 2.3: Update the lifecycle of CompareCache → (V3) Part 2.3: Update the lifecycle of CompareCache
Attachment #8865749 - Flags: review+
Attachment #8865750 - Attachment description: Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache → (V4) Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache
Attachment #8865750 - Flags: review+
Since it's not so big a modification, hope you don't mind that I update this patch directly,
Attachment #8864251 - Attachment is obsolete: true
Attachment #8865872 - Flags: review+
(In reply to Ben Hsu [:HoPang] from comment #70)
> Since it's not so big a modification, hope you don't mind that I update this
> patch directly,

Looks good.  Thanks.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e40aeb64bdab
Part 1.1: Move some function implementations out of CompareManager. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/fedca2ad4ce6
Part 1.2: Get the script URL list before actual comparing. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda24c8eccc4
Part 2.1: Move some functions out of their class definitions. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/974b55cde2af
Part 2.2: Make CompareManager::ComparisonFinished() public. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b2e0d54e51
Part 2.3: Update the lifecycle of CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb76b58e45fc
Part 2.4: Add mIsMainScript to CompareNetwork. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b85669af94
Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/7236a196cff0
Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e36f20bea04
Part 2.7: Unify how CompareManager, CompareNetwork and CompareCahce report errors during initialization and other operations. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a722c9de441
Part 2.8: Remove CompareManager::SetMaxScope() and CompareManager::SaveLoadFlags(). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6194a6a0f510
Part 3.1: Extend the bytecheck to imported scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fdb0ae64a1b
Part 3.2: Tolerate missing imported scripts. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2684d41c75e4
Part 4.1: Add a new wpt test for extended bytecheck. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cb7e835d4c
Part 4.2: Add a mochitest for an useless imported script after an update. r=bkelly.
Keywords: checkin-needed
Depends on: 1364059
Backed out for causing bug 1364059 (which was insta-crashing GMail).
https://hg.mozilla.org/mozilla-central/rev/4c580a771776f77c667fd457e2915568c2fcd0a7
Status: RESOLVED → REOPENED
Flags: needinfo?(bhsu)
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
Bonus points if we can land a test that would have caught this crash, BTW.
It would be great if we can add a crash test for this case.
I can't immediately reproduce the crash on gmail.  When I log in I don't get any service workers at all.  I think what is happening is that gmail experimented with service workers at one point and just left them installed.  The scripts are no longer hosted, though, so the update fetches fail.

This would be consistent with the crash stack.

I'll leave it to HoPang to investigate further.
It appears the service worker that is triggering this is:

  https://plus.google.com/serviceworker.js

This script 404s now.  It seems google just abandoned it on clients instead of unregistering it.
Attached file asan report
I believe this patch caused a AddressSanitizer: heap-use-after-free error on a number of web sites which was caught by bughunter though you folks have already reverted it. Attatching the asan output for one of them. Let me know if you need anything else.
Thanks again, I'll working on the testcase first this time.
Flags: needinfo?(bhsu)
Attachment #8840373 - Attachment is obsolete: true
Attachment #8840374 - Attachment is obsolete: true
Attachment #8855651 - Attachment is obsolete: true
Attachment #8855653 - Attachment is obsolete: true
Attachment #8855657 - Attachment is obsolete: true
Attachment #8855662 - Attachment is obsolete: true
Attachment #8857787 - Attachment is obsolete: true
Attachment #8864252 - Attachment is obsolete: true
Attachment #8864253 - Attachment is obsolete: true
Attachment #8864254 - Attachment is obsolete: true
Attachment #8864258 - Attachment is obsolete: true
Attachment #8865749 - Attachment is obsolete: true
Attachment #8865750 - Attachment is obsolete: true
Attachment #8865872 - Attachment is obsolete: true
CompareNetwork and CompareCache. r=bkelly
Attachment #8870667 - Attachment description: P2.1: Simplify the relationship between CompareManager, → P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache
Hi Ben, and sorry for doing a terrible job here.

The use-after-free crash is caused by two major facts, unexpected error and falsy implementation. I thought the previous revision can be covered by the test in P4.2 after unifying how related classes report error, but it turned out extremely unwell. In the previous revision, though the unexpected error is handled just like the other failures, it fails at the very last step, where a newly created CompareNetwork is assigned to a ReftPtr after being destructed. Since the unexpected error occurs within the constructor of CompareNetwork and the error handling make it ref-count drop to zero, and as a result, an use-after-free occurs.

After thinking over what I should do, I came out some tasks. Firstly, I dug into Necko and upgraded the testcase in P4.1, where the testcase can definitely crash the previous revision. Then, at the beginning, I tried to fix several stuff in the previous revision, but it ended up with rewriting them all since the error handling for "initialization" and "following async callback" should be separated, and remove some overly strict state-checks for concurrent loading, and thus this revision is able to stand the unexpected error without crashing.

After digging into Necko, I found out that the script loading flags should be updated, since otherwise cross-origin scripts can never being loaded. However, doing this eliminates the unexpected failure, but it also makes the falsy error handling remains unchecked again. Do you have any suggestion for this?

At last, hoping you don' mind reviewing them again.

Patch 1.X: Merely get the URL lists before doing byte-check.
           (move opening the old cache to CompareManager)
Patch 2.X: Simplify the relationship between CompareManager, CompareNetwork, and CompareCahce simpler
           (the main idea is to reduce the usage of mManager).
           Change the relationship between CompareManager, CompareNetwork and CcompareCache.
Patch 3.X: Extend bytecheck to imported scripts.
           (The script load settings are updated)
Patch 4.X: Add a wpt testcase for general cases
           (Add more testcase for importing cross-origin scripts)
           Add a mochitest for an edge case.
Flags: needinfo?(bkelly)
I'm sorry, but do you have diffs just from the last thing we landed?  Or do I need to re-review the entire patchset again?  Its really not clear to me what is new here vs what I've already reviewed.

Also, my review queue is open so you can flag r? now.

Thanks.
Flags: needinfo?(bkelly)
Attached patch temp.patch (obsolete) — Splinter Review
/* This is not a patch!!! */
Attachment #8870669 - Attachment is obsolete: true
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #92)
> I'm sorry, but do you have diffs just from the last thing we landed?  Or do
> I need to re-review the entire patchset again?  Its really not clear to me
> what is new here vs what I've already reviewed.

Since I've made a wrong decision in an very early patch of the previous design, "Not letting FetchScript() return anything.", making it return and moving related error handling make it's extremely hard to rebase. As a result I redo those patches, and check the error handling patch by patch. In fact, I don't know whether the best practice here is to provide followup patches or rewrite the patches again, which I have no strong preference on. If you don't against review the new patches, I'll provide a diff with comments between the two designs as a reference. However, do please feel free to cancel the reviews, if you think providing followup patches is the better option here, and I'll try to make the difference between the two designs as minimal and meaningful as possible. 

BTW, P4.2 remains untouched.
Attachment #8870665 - Flags: review?(bkelly)
Attachment #8870666 - Flags: review?(bkelly)
Attachment #8870667 - Flags: review?(bkelly)
Attachment #8870668 - Flags: review?(bkelly)
Attachment #8870670 - Flags: review?(bkelly)
Attachment #8870671 - Flags: review?(bkelly)
Attachment #8870672 - Flags: review?(bkelly)
Attachment #8870673 - Flags: review?(bkelly)
Attachment #8872267 - Flags: review?(bkelly)
Attachment #8870665 - Flags: review?(bkelly) → review+
Comment on attachment 8870666 [details] [diff] [review]
P1.2: Get the script URL list before actual comparing

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

I'm sorry, but I'm having a really hard time understanding this patch.  The description says:

"Get the script URL list before actual comparing"

But it seems to be doing a lot more than that.  There are many more states across two different objects.  Initialize() no long takes a CacheName which seems unrelated to the script URL. etc

Can you write out a description of how the state machine works now and the expected flow of the method calls and reflag for review?
Attachment #8870666 - Flags: review?(bkelly)
Comment on attachment 8870666 [details] [diff] [review]
P1.2: Get the script URL list before actual comparing

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

I spent some more time looking at this and I think I understand it a bit more.  The previously existing structure of the code just makes it very hard to review.  At some point in the future we should split these classes into separate files.  Its very hard to tell from the diff which class each method is associated with.

Overall it looks reasonable.  I do think we should use MOZ_DIAGNOSTIC_ASSERT() for the state machine, though.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +359,5 @@
>    void
>    Cleanup();
>  
> +  nsresult
> +  FetchScript(const nsAString& aURL, Cache* const aCache = nullptr)

Can you MOZ_DIAGNOSTIC_ASSERT() mState here?

@@ +361,5 @@
>  
> +  nsresult
> +  FetchScript(const nsAString& aURL, Cache* const aCache = nullptr)
> +  {
> +    mCN = new CompareNetwork(this);

Can you MOZ_DIAGNOSTIC_ASSERT(!mCN) here?  Or can these get overwritten?  If so, how is the previous one cleaned up if its in process?

@@ +368,5 @@
> +      return rv;
> +    }
> +
> +    if (aCache) {
> +      mCC = new CompareCache(this);

Can you MOZ_DIAGNOSTIC_ASSERT(!mCC) here?  Same questions about overwriting.

@@ +371,5 @@
> +    if (aCache) {
> +      mCC = new CompareCache(this);
> +      rv = mCC->Initialize(aCache, aURL);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        mCN->Abort();

Maybe do this cleanup via MakeScopeExit?
Attachment #8870666 - Flags: review+
Comment on attachment 8870667 [details] [diff] [review]
P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache

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

It appears this patch is mainly trying to limit the use of mManager throughout CompareNetwork and CompareCache.  Looks reasonable.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +634,4 @@
>    MOZ_ASSERT(aPrincipal);
>    AssertIsOnMainThread();
>  
> +  mURL = aURL;

Can we initialize mURL after we verify it parses correctly below?

@@ +742,5 @@
> +nsresult
> +CompareNetwork::SetPrincipalInfo(nsIChannel* aChannel)
> +{
> +  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +  NS_ASSERTION(ssm, "Should never be null!");

Please don't use NS_ASSERTION().  Either use a hard assertion like MOZ_DIAGNOSTIC_ASSERT() or return failure.  Since you have an nsresult error code you can just return the failure.
Attachment #8870667 - Flags: review?(bkelly) → review+
Comment on attachment 8870668 [details] [diff] [review]
P2.2: Make CompareManager::ComparisonFinished()

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

I think the commit message is missing a word?  Is it supposed to say "Make CompareManager::ComparisonFinished() public"?
Attachment #8870668 - Flags: review?(bkelly) → review+
Comment on attachment 8870672 [details] [diff] [review]
P4.1: Add a new wpt test for extended bytecheck

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

Nice!

::: testing/web-platform/tests/service-workers/service-worker/update-bytecheck.https.html
@@ +6,5 @@
> +<script src="/resources/testharnessreport.js"></script>
> +<script src="resources/test-helpers.sub.js"></script>
> +<script>
> +
> +const settings = [{cors: false, main: 'default', imported: 'default'},

Can you add a comment about what these settings mean?
Attachment #8870672 - Flags: review?(bkelly) → review+
Comment on attachment 8870673 [details] [diff] [review]
P4.2: Add a mochitest for an useless imported script after an update

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

Can you rename these files to something more human readable?  I find it hard to work with tests that only have bug numbers in them.

::: dom/workers/test/serviceworkers/bug1290951_worker_main.sjs
@@ +7,5 @@
> +  importScripts("bug1290951_worker_imported.sjs");
> +`;
> +
> +const WORKER_2 = `
> +  // Remove importScripts(...)" for testing purpose.

What does this comment mean?

::: dom/workers/test/serviceworkers/test_bug1290951.html
@@ +25,5 @@
> +      .then(_ => navigator.serviceWorker.register("http://mochi.test:8888/" +
> +                                                  "tests/dom/workers/test/" +
> +                                                  "serviceworkers/" +
> +                                                  "bug1290951_worker_main.sjs"))
> +      .then(r => ok(r, "Should be a registration."));

Maybe wait for active state before proceeding here?  Maybe not necessary, but it ensures the test runs the same way each time.
Attachment #8870673 - Flags: review?(bkelly) → review+
Sorry, I will have to review the last two patches tomorrow.
Comment on attachment 8872267 [details] [diff] [review]
P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +111,5 @@
> +  Initialize(nsIPrincipal* aPrincipal,
> +             const nsAString& aURL,
> +             nsILoadGroup* aLoadGroup,
> +             Cache* const aCache);
> +  

nit: trailing whitespace

@@ +819,5 @@
>    }
>  
>    if (NS_WARN_IF(NS_FAILED(aStatus))) {
> +    bool hide = aStatus == NS_ERROR_REDIRECT_LOOP;
> +    NetworkFinish(hide ? NS_ERROR_DOM_SECURITY_ERR : aStatus);

Can you make CompareNetwork::OnStreamComplete() use MakeScopeExit() for the error reporting cases?
Attachment #8872267 - Flags: review?(bkelly) → review+
Comment on attachment 8870670 [details] [diff] [review]
P3.1: Extend bytecheck to imported scripts

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +509,4 @@
>  
>      // Just to be safe.
>      RefPtr<Cache> kungfuDeathGrip = cache;
> +    mState = WaitingForPut;

Maybe set the WaitingForPut state after successfully start all the put operations?

@@ +510,5 @@
>      // Just to be safe.
>      RefPtr<Cache> kungfuDeathGrip = cache;
> +    mState = WaitingForPut;
> +
> +    MOZ_ASSERT(mPendingCount == 0);

Just to make sure I understand, we wait for all individual scripts to fetch and compare to the cache.  Only then we write them to the new cache?  So we have to hold them all in memory for some time?

Also, do we require all fetches to complete before we will compare any of them to cache?  Or can individual scripts compare to their cache as soon as their fetch is complete?

Finally, if we do allow things to compare early, do we actually cancel the other ongoing fetches?

At this point I'm just curious.  These would be optimizations and could be left follow-on bugs.

@@ +605,4 @@
>    JS::PersistentRooted<JSObject*> mSandbox;
>    RefPtr<CacheStorage> mCacheStorage;
>  
> +  nsTArray<RefPtr<CompareNetwork>> mCNs;

nit: I think its a little easier to read mCNList vs the plural 's'.  When you have variables that differ by only one letter it can get confusing.
Attachment #8870670 - Flags: review?(bkelly) → review+
Attachment #8870671 - Flags: review?(bkelly) → review+
I think state machine things in general would be good to put in MOZ_DIAGNOSTIC_ASSERT().

Also, can you post a full try build before landing?  I'd like to download the build and run with it a bit.  Thanks!
Comment on attachment 8870666 [details] [diff] [review]
P1.2: Get the script URL list before actual comparing

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +359,5 @@
>    void
>    Cleanup();
>  
> +  nsresult
> +  FetchScript(const nsAString& aURL, Cache* const aCache = nullptr)

Sure!

@@ +361,5 @@
>  
> +  nsresult
> +  FetchScript(const nsAString& aURL, Cache* const aCache = nullptr)
> +  {
> +    mCN = new CompareNetwork(this);

Being protected by the URL comparison before invoking this method, this method is invoked only one time, so mCN and mCC can't be overwritten.

@@ +368,5 @@
> +      return rv;
> +    }
> +
> +    if (aCache) {
> +      mCC = new CompareCache(this);

Same as above.

@@ +371,5 @@
> +    if (aCache) {
> +      mCC = new CompareCache(this);
> +      rv = mCC->Initialize(aCache, aURL);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        mCN->Abort();

I'd prefer not to, since doing RAII here only help two cases and they should be handled differently.
Comment on attachment 8870667 [details] [diff] [review]
P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +634,4 @@
>    MOZ_ASSERT(aPrincipal);
>    AssertIsOnMainThread();
>  
> +  mURL = aURL;

Sure!

@@ +742,5 @@
> +nsresult
> +CompareNetwork::SetPrincipalInfo(nsIChannel* aChannel)
> +{
> +  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +  NS_ASSERTION(ssm, "Should never be null!");

Updated!
Comment on attachment 8870670 [details] [diff] [review]
P3.1: Extend bytecheck to imported scripts

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +509,4 @@
>  
>      // Just to be safe.
>      RefPtr<Cache> kungfuDeathGrip = cache;
> +    mState = WaitingForPut;

Sure!

@@ +510,5 @@
>      // Just to be safe.
>      RefPtr<Cache> kungfuDeathGrip = cache;
> +    mState = WaitingForPut;
> +
> +    MOZ_ASSERT(mPendingCount == 0);

Q1:
Yes, we have to hold those CompareNetworks until the comparison is finished, but the CompareCaches are release once the comparison of an individual script is done.

Q2:
A script is compared immediately once its corresponding CompareNetwork and CompareCache are ready. After then, the CompareCache is released.

Q3:
IMHO, that is one thing that we can to be optimized. If one different script is found, we can cancel ongoing fetches and start the new worker with the new main script.

@@ +605,4 @@
>    JS::PersistentRooted<JSObject*> mSandbox;
>    RefPtr<CacheStorage> mCacheStorage;
>  
> +  nsTArray<RefPtr<CompareNetwork>> mCNs;

Updated
Comment on attachment 8870672 [details] [diff] [review]
P4.1: Add a new wpt test for extended bytecheck

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

::: testing/web-platform/tests/service-workers/service-worker/update-bytecheck.https.html
@@ +6,5 @@
> +<script src="/resources/testharnessreport.js"></script>
> +<script src="resources/test-helpers.sub.js"></script>
> +<script>
> +
> +const settings = [{cors: false, main: 'default', imported: 'default'},

Updated!
Comment on attachment 8870673 [details] [diff] [review]
P4.2: Add a mochitest for an useless imported script after an update

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

::: dom/workers/test/serviceworkers/bug1290951_worker_main.sjs
@@ +7,5 @@
> +  importScripts("bug1290951_worker_imported.sjs");
> +`;
> +
> +const WORKER_2 = `
> +  // Remove importScripts(...)" for testing purpose.

I was trying to say that there is no imported script anymore in this version.

::: dom/workers/test/serviceworkers/test_bug1290951.html
@@ +25,5 @@
> +      .then(_ => navigator.serviceWorker.register("http://mochi.test:8888/" +
> +                                                  "tests/dom/workers/test/" +
> +                                                  "serviceworkers/" +
> +                                                  "bug1290951_worker_main.sjs"))
> +      .then(r => ok(r, "Should be a registration."));

Updated.
Comment on attachment 8872267 [details] [diff] [review]
P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +111,5 @@
> +  Initialize(nsIPrincipal* aPrincipal,
> +             const nsAString& aURL,
> +             nsILoadGroup* aLoadGroup,
> +             Cache* const aCache);
> +  

updated

@@ +819,5 @@
>    }
>  
>    if (NS_WARN_IF(NS_FAILED(aStatus))) {
> +    bool hide = aStatus == NS_ERROR_REDIRECT_LOOP;
> +    NetworkFinish(hide ? NS_ERROR_DOM_SECURITY_ERR : aStatus);

Updated
Attachment #8872251 - Attachment is obsolete: true
Attachment #8870666 - Attachment is obsolete: true
Attachment #8870667 - Attachment is obsolete: true
Attachment #8870668 - Attachment is obsolete: true
Attachment #8870670 - Attachment is obsolete: true
Attachment #8870672 - Attachment is obsolete: true
Attachment #8870673 - Attachment is obsolete: true
Attachment #8872267 - Attachment is obsolete: true
Attachment #8870671 - Attachment is obsolete: true
Attachment #8875173 - Attachment description: P3.2: Tolerate missing imported scripts → (V2) P3.2: Tolerate missing imported scripts
Attachment #8875145 - Flags: review+
Attachment #8875146 - Flags: review+
Attachment #8875148 - Flags: review+
Attachment #8875149 - Flags: review+
Attachment #8875150 - Flags: review+
Attachment #8875152 - Flags: review?(bkelly)
Attachment #8875153 - Flags: review+
Attachment #8875155 - Flags: review+
Attachment #8875173 - Flags: review+
(In reply to Ben Hsu [:HoPang] from comment #108)
> Q3:
> IMHO, that is one thing that we can to be optimized. If one different script
> is found, we can cancel ongoing fetches and start the new worker with the
> new main script.

Can you write a follow-up bug to do this and make it block the ServiceWorkers-perf meta bug?  Thanks!
Comment on attachment 8875152 [details] [diff] [review]
P3.3: Move state checks to MOZ_DIAGNOSTIC_ASSERT()

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

Thanks!
Attachment #8875152 - Flags: review?(bkelly) → review+
I'm running with the try build.  So far so good!
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #124)
> I'm running with the try build.  So far so good!

Would be good to run longer in nightly cycle. So, time to push to nightly?
Yea, I think we should go ahead and land this again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=488db51263a83a7f00776e2474b15e130a1bb8c4

The test is provided via updating P4.1 which covers cross-origin scripts now.
Keywords: checkin-needed
For some reason, your patch author information is inconsistently going back and forth between "Ho-Pang Hsu <hopang.hsu@gmail.com>" and "Ho-Pang <hopang.hsu@gmail.com>". Do you have any idea why?
Flags: needinfo?(bhsu)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7c1fe60de
P1.1: Move some function implementations out of their class definitions. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fcec2a3c03
P1.2: Change the lifecycle of CompareManager and CompareCache to get the script URL list before actual comparing. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a69cc55f028
P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa2434feb65
P2.2: Make CompareManager::ComparisonFinished() public. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba6fe3c40912
P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b07bbf8547
P3.1: Extend bytecheck to imported scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b397d679eef
P3.2: Tolerate missing imported scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca966441d114
P3.3: Move state checks to MOZ_DIAGNOSTIC_ASSERT(). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ef74ca277b
P4.1: Add a new wpt test for extended bytecheck. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/37a5a5c1090f
P4.2: Add a mochitest for an useless imported script after an update. r=bkelly
Keywords: checkin-needed
My apologies, it seems that my home computer and my working laptop have different settings... I've update the settings...
Flags: needinfo?(bhsu)
Depends on: 1424300
You need to log in before you can comment on or make changes to this bug.