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

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 2

[Ben Hsu [:HoPang]]

As [0] suggest, we should take care double-download issue in this bug.

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1290944#c55

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 1.1: Move some function implementations out of

CompareManager. r=bkelly

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 1.1: Move some function implementations out of CompareManager

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 1.2: Get the script URL list before actual comparing

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 2.1: Move some functions out of their class definitions

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 2.2: Make CompareManager::ComparisonFinished() public

Comment 8

[Ben Hsu [:HoPang]]

Attachment: Part 2.3: Update the lifecycle of CompareCache

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 2.4: Add mIsMainScript to CompareNetwork

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2.4: Add mIsMainScript to CompareNetwork

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Comment 13

[Ben Hsu [:HoPang]]

Attachment: Part 3.1: Extend the bytecheck to imported scripts

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Part 3.2: Tolerate missing imported scripts

Comment 15

[Ben Hsu [:HoPang]]

Attachment: Part 4.1: Add a new wpt test for extended bytecheck

Comment 16

[Ben Hsu [:HoPang]]

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 18

[Ben Hsu [:HoPang]]

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 20

[Ben Hsu [:HoPang]]

Attachment: Part 3.2: Tolerate missing imported scripts

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 4.2: Add a mochitest for an useless imported script after an update

Comment 22

[Ben Hsu [:HoPang]]

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.

Comment 23

[Ben Hsu [:HoPang]]

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

Comment 24

[Andrea Marchesini [:baku]]

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.

Comment 25

[Ben Hsu [:HoPang]]

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

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

I'm going to try to review this since I'm sitting at the same table with Ho-Pang this week.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 35

[Ben Hsu [:HoPang]]

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 36

[Ben Hsu [:HoPang]]

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 37

[Ben Hsu [:HoPang]]

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?

Comment 38

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 1.1: Move some function implementations out of CompareManager

Update the reviewer id.

Comment 39

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 1.2: Get the script URL list before actual comparing

Updated

Comment 40

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork

Comment 41

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3.1: Extend the bytecheck to imported scripts

Comment 42

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.3: Update the lifecycle of CompareCache

Comment 43

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.4: Add mIsMainScript to CompareNetwork

Comment 44

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Comment 45

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 4.1: Add a new wpt test for extended bytecheck

Comment 46

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 4.2: Add a mochitest for an useless imported script after an update

Comment 47

[Ben Hsu [:HoPang]]

Hmm, the try result doesn't seem well. There is memory leaked...

Comment 48

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.5: Move ChannelInfo and PrincipalInfo into CompareNetwork

Fix the memory leak mentioned in comment 47.

Comment 49

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Fix the memory leak mentioned in comment 47.

Comment 50

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 3.1: Extend the bytecheck to imported scripts

Fix the memory leak mentioned in comment 47.

Comment 51

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13ac36eaa59da1d3718b7c8a8627112de26361a4

Comment 52

[Pulsebot]

Pushed by [e-mail]:
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

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d518009bfe70
https://hg.mozilla.org/mozilla-central/rev/a459d48f1d86
https://hg.mozilla.org/mozilla-central/rev/6f30a5669251
https://hg.mozilla.org/mozilla-central/rev/437aa650b9d9
https://hg.mozilla.org/mozilla-central/rev/779deac689f9
https://hg.mozilla.org/mozilla-central/rev/9e1c5c05eb58
https://hg.mozilla.org/mozilla-central/rev/a5a8e0001d78
https://hg.mozilla.org/mozilla-central/rev/2e133f3b768c
https://hg.mozilla.org/mozilla-central/rev/a4b47c4e9e3b
https://hg.mozilla.org/mozilla-central/rev/5be709822325
https://hg.mozilla.org/mozilla-central/rev/2793d9e7c1d2
https://hg.mozilla.org/mozilla-central/rev/c351046112bf

Comment 54

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e for causing Bug 1357437

Comment 55

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5a3776a6e4f8
Backed out changeset c351046112bf 
https://hg.mozilla.org/mozilla-central/rev/930e3b959c7a
Backed out changeset 2793d9e7c1d2 
https://hg.mozilla.org/mozilla-central/rev/8d4b9f6e0d43
Backed out changeset 5be709822325 
https://hg.mozilla.org/mozilla-central/rev/680258e06fbf
Backed out changeset a4b47c4e9e3b 
https://hg.mozilla.org/mozilla-central/rev/f932fd427c88
Backed out changeset 2e133f3b768c 
https://hg.mozilla.org/mozilla-central/rev/fb0749acbab6
Backed out changeset a5a8e0001d78 
https://hg.mozilla.org/mozilla-central/rev/0f9f397b9485
Backed out changeset 9e1c5c05eb58 
https://hg.mozilla.org/mozilla-central/rev/95798deaebd9
Backed out changeset 779deac689f9 
https://hg.mozilla.org/mozilla-central/rev/b429f1d4b4db
Backed out changeset 437aa650b9d9 
https://hg.mozilla.org/mozilla-central/rev/4cd8f9af2291
Backed out changeset 6f30a5669251 
https://hg.mozilla.org/mozilla-central/rev/6d498f6de1fc
Backed out changeset a459d48f1d86

Comment 56

[Ben Hsu [:HoPang]]

Thanks, I'll check them again.

Comment 57

[Ben Hsu [:HoPang]]

Attachment: Part 2.7: Unify how CompareManager, CompareNetwork and CompareCahce report errors during initialization and other operations

Comment 58

[Ben Hsu [:HoPang]]

Attachment: Part 2.8: Remove CompareManager::SetMaxScope() and CompareManager::SaveLoadFlags()

Comment 59

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 3.1: Extend the bytecheck to imported scripts

Comment 60

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3.2: Tolerate missing imported scripts

Comment 61

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 4.1: Add a new wpt test for extended bytecheck

Rebased

Comment 62

[Ben Hsu [:HoPang]]

Comment on attachment 8864258 [details] [diff] [review]
(V3) Part 4.1: Add a new wpt test for extended bytecheck

Rebased

Comment 63

[Ben Hsu [:HoPang]]

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.

Comment 64

[Ben Hsu [:HoPang]]

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 65

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 66

[Ben Hsu [:HoPang]]

Comment on attachment 8864253 [details] [diff] [review]
(V4) Part 3.1: Extend the bytecheck to imported scripts

Rebased version.

Comment 67

[Ben Hsu [:HoPang]]

Comment on attachment 8864254 [details] [diff] [review]
(V2) Part 3.2: Tolerate missing imported scripts

Rebased version.

Comment 68

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.3: Update the lifecycle of CompareCache

Rebased - Fastforward

Comment 69

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 2.6: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Rebased - Fastforward

Comment 70

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.7: Unify how CompareManager, CompareNetwork and CompareCahce report errors during initialization and other operations

Since it's not so big a modification, hope you don't mind that I update this patch directly,

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 72

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=947c92bc74b283ac71e27c392ea657526683e56f&selectedJob=97917671

Comment 73

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e40aeb64bdab
https://hg.mozilla.org/mozilla-central/rev/fedca2ad4ce6
https://hg.mozilla.org/mozilla-central/rev/cda24c8eccc4
https://hg.mozilla.org/mozilla-central/rev/974b55cde2af
https://hg.mozilla.org/mozilla-central/rev/f2b2e0d54e51
https://hg.mozilla.org/mozilla-central/rev/bb76b58e45fc
https://hg.mozilla.org/mozilla-central/rev/b6b85669af94
https://hg.mozilla.org/mozilla-central/rev/7236a196cff0
https://hg.mozilla.org/mozilla-central/rev/4e36f20bea04
https://hg.mozilla.org/mozilla-central/rev/9a722c9de441
https://hg.mozilla.org/mozilla-central/rev/6194a6a0f510
https://hg.mozilla.org/mozilla-central/rev/2fdb0ae64a1b
https://hg.mozilla.org/mozilla-central/rev/2684d41c75e4
https://hg.mozilla.org/mozilla-central/rev/d9cb7e835d4c

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1364059 (which was insta-crashing GMail).
https://hg.mozilla.org/mozilla-central/rev/4c580a771776f77c667fd457e2915568c2fcd0a7

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Bonus points if we can land a test that would have caught this crash, BTW.

Comment 77

[Astley Chen (inactive)]

It would be great if we can add a crash test for this case.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 80

[Bob Clary [:bc] (inactive)]

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

Comment 81

[Ben Hsu [:HoPang]]

Thanks again, I'll working on the testcase first this time.

Comment 82

[Ben Hsu [:HoPang]]

Attachment: P1.1: Move some function implementations out of their class definitions

Comment 83

[Ben Hsu [:HoPang]]

Attachment: P1.2: Get the script URL list before actual comparing

Comment 84

[Ben Hsu [:HoPang]]

Attachment: P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache

CompareNetwork and CompareCache. r=bkelly

Comment 85

[Ben Hsu [:HoPang]]

Attachment: P2.2: Make CompareManager::ComparisonFinished()

Comment 86

[Ben Hsu [:HoPang]]

Attachment: P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Comment 87

[Ben Hsu [:HoPang]]

Attachment: P3.1: Extend bytecheck to imported scripts

Comment 88

[Ben Hsu [:HoPang]]

Attachment: P3.2: Tolerate missing imported scripts

Comment 89

[Ben Hsu [:HoPang]]

Attachment: P4.1: Add a new wpt test for extended bytecheck

Comment 90

[Ben Hsu [:HoPang]]

Attachment: P4.2: Add a mochitest for an useless imported script after an update

Comment 91

[Ben Hsu [:HoPang]]

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.

Comment 92

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 93

[Ben Hsu [:HoPang]]

Attachment: temp.patch

/* This is not a patch!!! */

Comment 94

[Ben Hsu [:HoPang]]

Attachment: P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Comment 95

[Ben Hsu [:HoPang]]

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

Comment 96

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 97

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 98

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 99

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 100

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 101

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 102

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I will have to review the last two patches tomorrow.

Comment 103

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 104

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 105

[Ben Kelly [:bkelly, not reviewing]]

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 106

[Ben Hsu [:HoPang]]

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 107

[Ben Hsu [:HoPang]]

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 108

[Ben Hsu [:HoPang]]

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 109

[Ben Hsu [:HoPang]]

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 110

[Ben Hsu [:HoPang]]

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 111

[Ben Hsu [:HoPang]]

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

Comment 112

[Ben Hsu [:HoPang]]

Attachment: (V2) P1.2: Change the lifecycle of CompareManager and CompareCache to get the script URL list before actual comparing

Comment 113

[Ben Hsu [:HoPang]]

Attachment: (V2) P2.1: Simplify the relationship between CompareManager, CompareNetwork and CompareCache

Comment 114

[Ben Hsu [:HoPang]]

Attachment: (V2) P2.2: Make CompareManager::ComparisonFinished() public

Comment 115

[Ben Hsu [:HoPang]]

Attachment: (V2) P2.3: Change the relationship between CompareManager, CompareNetwork, and CompareCache

Comment 116

[Ben Hsu [:HoPang]]

Attachment: (V2) P3.1: Extend bytecheck to imported scripts

Comment 117

[Ben Hsu [:HoPang]]

Attachment: P3.3: Move state checks to MOZ_DIAGNOSTIC_ASSERT()

Comment 118

[Ben Hsu [:HoPang]]

Attachment: (V2) P4.1: Add a new wpt test for extended bytecheck

Comment 119

[Ben Hsu [:HoPang]]

Attachment: (V2) P4.2: Add a mochitest for an useless imported script after an update

Comment 120

[Ben Hsu [:HoPang]]

Attachment: (V2) P3.2: Tolerate missing imported scripts

Rebased

Comment 121

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875152 [details] [diff] [review]
P3.3: Move state checks to MOZ_DIAGNOSTIC_ASSERT()

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

Thanks!

Comment 123

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49f55d40feb0c2521c05f7a75f841f9abd5dd415

Here is the try build.

Comment 124

[Ben Kelly [:bkelly, not reviewing]]

I'm running with the try build.  So far so good!

Comment 125

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 126

[Ben Kelly [:bkelly, not reviewing]]

Yea, I think we should go ahead and land this again.

Comment 127

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=488db51263a83a7f00776e2474b15e130a1bb8c4

The test is provided via updating P4.1 which covers cross-origin scripts now.

Comment 128

[Ryan VanderMeulen [:RyanVM]]

For some reason, your patch author information is inconsistently going back and forth between "Ho-Pang Hsu <[e-mail]>" and "Ho-Pang <[e-mail]>". Do you have any idea why?

Comment 129

[Pulsebot]

Pushed by [e-mail]:
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

Comment 130

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4be7c1fe60de
https://hg.mozilla.org/mozilla-central/rev/e8fcec2a3c03
https://hg.mozilla.org/mozilla-central/rev/0a69cc55f028
https://hg.mozilla.org/mozilla-central/rev/5fa2434feb65
https://hg.mozilla.org/mozilla-central/rev/ba6fe3c40912
https://hg.mozilla.org/mozilla-central/rev/e8b07bbf8547
https://hg.mozilla.org/mozilla-central/rev/8b397d679eef
https://hg.mozilla.org/mozilla-central/rev/ca966441d114
https://hg.mozilla.org/mozilla-central/rev/a6ef74ca277b
https://hg.mozilla.org/mozilla-central/rev/37a5a5c1090f

Comment 131

[Ben Hsu [:HoPang]]

My apologies, it seems that my home computer and my working laptop have different settings... I've update the settings...