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!

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

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!

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

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.

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

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.

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

Updated

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.

Fix the memory leak mentioned in comment 47.

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

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

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

Backout by cbook@mozilla.com: 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

Thanks, I'll check them again.

Rebased

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

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.

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.

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

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

Rebased - Fastforward

Rebased - Fastforward

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

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

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

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

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.

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.

CompareNetwork and CompareCache. r=bkelly

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.

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.

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

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

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?

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.

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!