Closed Bug 1154494 Opened 9 years ago Closed 9 years ago

ServiceWorkerScriptCache should write new worker script to cache.

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: nsm, Assigned: nsm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Hit network only once
15.79 KB, patch
ehsan.akhgari
: review+
baku
: review+
Details | Diff | Splinter Review
patch 2 - Hold a ref to the sandbox
2.71 KB, patch
baku
: review+
Details | Diff | Splinter Review
Attached patch The fix (obsolete) — — Splinter Review 
When the network script is newer, it should be written to a new cache, otherwise creating a worker leads to another network request. This is inefficient and also affects our testing.

I don't have a test for this because the test for Bug 1112469 exercises this.
Attachment #8592497 - Attachment is patch: true
Attachment #8592497 - Flags: review?(bkelly)
Attachment #8592497 - Flags: review?(amarchesini)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #0)
> I don't have a test for this because the test for Bug 1112469 exercises this.

Note that I have a couple of todo_is's in that test which should be flipped to is's when this lands.
I will do this review tomorrow morning.  Sorry it has taken so long!
Comment on attachment 8592497 [details] [diff] [review]
The fix

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

Looks good.  r=me with comments.  Thanks!

::: dom/workers/ScriptLoader.cpp
@@ +790,5 @@
>        nsRefPtr<InternalResponse> ir =
>          new InternalResponse(200, NS_LITERAL_CSTRING("OK"));
>        ir->SetBody(reader);
>  
> +      //XXXnsm: Is it bad that we don't cache the original headers?

The script loader never used to look at any headers in the nsIChannel, did it?

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +288,5 @@
> +    // the cache even if there isn't an existing one.
> +    ErrorResult result;
> +    mCacheStorage = CreateCacheStorage(aPrincipal, result);
> +    if (NS_WARN_IF(result.Failed())) {
> +      mCN->Abort();

Would it be easier to create the CacheStorage first so you don't have to abort the network comparison?

@@ +307,5 @@
>  
> +  const nsAString&
> +  URL() const
> +  {
> +    return mURL;

You assert main thread in the other URL() getter above.  Does that apply here as well?

@@ +397,5 @@
> +
> +      // Just to be safe.
> +      nsRefPtr<Cache> kungfuDeathGrip = cache;
> +      WriteToCache(cache);
> +    } else {

Early return and then just drop the else?

@@ +436,5 @@
>    void
> +  Fail(nsresult aStatus)
> +  {
> +    AssertIsOnMainThread();
> +    MOZ_ASSERT(mCallback);

You don't need this assert as nsRefPtr<> will assert on nullptr below automatically.

@@ +437,5 @@
> +  Fail(nsresult aStatus)
> +  {
> +    AssertIsOnMainThread();
> +    MOZ_ASSERT(mCallback);
> +    mCallback->ComparisonResult(aStatus, false, EmptyString());

/* aIsEqual */ on the boolean?

@@ +446,5 @@
> +  Cleanup()
> +  {
> +    AssertIsOnMainThread();
> +    mCallback = nullptr;
> +    mCN = nullptr;

Can this be double called?  If not, MOZ_ASSERT(mCallback) here?

@@ +459,5 @@
>  
> +    if (aIsEqual) {
> +      mCallback->ComparisonResult(aStatus, aIsEqual, EmptyString());
> +      Cleanup();
> +    } else {

Early return and remove else?

@@ +476,5 @@
> +
> +    ErrorResult result;
> +    result = serviceWorkerScriptCache::GenerateCacheName(mNewCacheName);
> +    if (NS_WARN_IF(result.Failed())) {
> +      Fail(result.ErrorCode());

MOZ_ASSERT(!result.IsErrorWithMessage())

This isn't going to work quite right if you have a TypeError or something.  So assert that we don't when getting ErrorCode().  Here and other places you use ErrorCode().

@@ +495,5 @@
> +  WriteToCache(Cache* aCache)
> +  {
> +    AssertIsOnMainThread();
> +    MOZ_ASSERT(aCache);
> +

MOZ_ASSERT(mState == WaitingForOpen)?
Attachment #8592497 - Flags: review?(bkelly) → review+
Comment on attachment 8592497 [details] [diff] [review]
The fix

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +155,5 @@
>  
>  // This class gets a cached Response from the CacheStorage and then it calls
>  // CacheFinished() in the CompareManager.
>  class CompareCache final : public PromiseNativeHandler
> +                         , public nsIStreamLoaderObserver

I think this is already fixed by another patch.

@@ +281,2 @@
>      mCN = new CompareNetwork(this);
> +    nsresult rv = mCN->Initialize(aPrincipal, URL());

mURL or aURL directly?

@@ +296,3 @@
>      if (!aCacheName.IsEmpty()) {
>        mCC = new CompareCache(this);
> +      rv = mCC->Initialize(aPrincipal, URL(), aCacheName);

URL() return what we have in aURL, can we use it directly?

@@ +390,5 @@
> +
> +      Cache* cache = nullptr;
> +      nsresult rv = UNWRAP_OBJECT(Cache, obj, cache);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        Fail(NS_ERROR_FAILURE);

Fail(rv) ?
Attachment #8592497 - Flags: review?(amarchesini) → review+
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 1156847
Noting down probable cause:
It is possible that we hit these failures - https://treeherder.mozilla.org/logviewer.html#?job_id=9246534&repo=mozilla-inbound
because when the ServiceWorkerScriptCache gets a 404 or something, it marks that as an error, but the error is not sent up via the worker error handling mechanism (the way ScriptLoader does by setting an exception), but by a direct call to ServiceWorkerRegisterJob. SWM::HandleError is expecting a registration failure from a js error.
Comment on attachment 8596921 [details] [diff] [review]
fail from comparisonresult

attached the wrong patch.
Attachment #8596921 - Attachment is obsolete: true
Attachment #8596921 - Flags: review?(amarchesini)
With the call to fail when ComparisonFinished is called with error status.
Attachment #8597445 - Flags: review?(ehsan)
Comment on attachment 8597445 [details] [diff] [review]
Hit network only once

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

I asked Nikhil to try to land this patch today because I need it to keep working on bug 1156847, but leaving a review request to baku so that he can look at the changes as well.
Attachment #8597445 - Flags: review?(ehsan)
Attachment #8597445 - Flags: review?(amarchesini)
Attachment #8597445 - Flags: review+
This gave me a build error, from the AddRef/Release methods declared via the macro here:

>diff --git a/dom/workers/ServiceWorkerScriptCache.cpp b/dom/workers/ServiceWorkerScriptCache.cpp
>-class CompareManager final
>+class CompareManager final : public PromiseNativeHandler
> {
> public:
>   NS_INLINE_DECL_REFCOUNTING(CompareManager)

I went ahead and added "override" to the macro, to satisfy the build warning (cset in comment 16 ^).

BUT, after landing, I realized these refcounting methods probably don't really intend to be overrides, since the superclass (PromiseNativeHandler) provides us with *actual* implementations (as opposed to pure virtual declarations). So, we don't need to implement them again here. So I think this 
NS_INLINE_DECL_REFCOUNTING macro might just need to be removed, really.

I tried deleting this NS_INLINE_DECL_REFCOUNTING macro locally, and I can still build just fine, which I think means we're OK to delete it.

needinfo=nsm to follow up on this.
Flags: needinfo?(nsm.nikhil)
(Looks like the other PromiseNativeHandler subclasses don't implement AddRef/Release, so this class (CompareManager) probably shouldn't either.  Also, from a safety perspective -- PromiseNativeHandler has a virtual destructor, so it should be fine for its subclasses to inherit its Release() method, & for that inherited method to be the thing that ends up deleting instances of the concrete subclasses.)
I backed this out (both nsm's original patch and dholbert's followup):
https://hg.mozilla.org/integration/mozilla-inbound/rev/5687f349da5e
for causing intermittent failures in a significant number of different service worker tests, which constituted a significant part of the intermittent orange today.

To see a sample of those failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=mochitest-4&fromchange=f53de79da7f2&tochange=d1269f811e05
Note that some of them got filed and some of them were never filed.

They include:
 486 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_periodic_update.html | Test timed out. - expected PASS 

 491 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_installation_simple.html | Test timed out. - expected PASS 

 389 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_installation_simple.html | getRegistrations returns an array with 1 item - expected PASS 

 366 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_empty_serviceworker.html | Test timed out. - expected PASS 

 365 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_controller.html | Some test failed with error TypeError: The expression cannot be converted to return the specified type. - expected PASS 

 383 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_match_all_client_id.html | Some test failed with error TypeError: The expression cannot be converted to return the specified type. - expected PASS 

 368 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_https_fetch.html | Test timed out. - expected PASS
Note that I think bug 1157901 is not related to this, but I think today's spike in bug 1134841 is, as is bug 1158530 (new today), as was today's occurrence of bug 1065134.
Comment on attachment 8597445 [details] [diff] [review]
Hit network only once

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

lgtm

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +155,5 @@
>  
>  // This class gets a cached Response from the CacheStorage and then it calls
>  // CacheFinished() in the CompareManager.
>  class CompareCache final : public PromiseNativeHandler
> +                         , public nsIStreamLoaderObserver

I think your tree is old.

@@ +282,5 @@
> +    // the cache even if there isn't an existing one.
> +    ErrorResult result;
> +    mCacheStorage = CreateCacheStorage(aPrincipal, result);
> +    if (NS_WARN_IF(result.Failed())) {
> +      MOZ_ASSERT(!result.IsErrorWithMessage());

why these assertions?

@@ +294,5 @@
>      }
>  
>      if (!aCacheName.IsEmpty()) {
>        mCC = new CompareCache(this);
> +      rv = mCC->Initialize(aPrincipal, aURL, aCacheName);

thanks for this :)
Attachment #8597445 - Flags: review?(amarchesini) → review+
Depends on: 1159378
Nikhil, if I fix bug 1156847 first, please make sure to land your patch here together with the patches 1159378, otherwise this will break tests, etc.
That is, "with the patches in bug 1159378..."
The sandbox does not hold on to its JS wrapper, so we need to hold a ref to the nsIXPConnectJSObjectHolder which roots the wrapper.
Attachment #8600967 - Flags: review?(amarchesini)
Attachment #8597445 - Attachment is obsolete: false
Flags: needinfo?(nsm.nikhil)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d9c016d4686

shows that updates are being requested (by the call to SoftUpdate). SoftUpdate is only called from the PeriodicUpdater or registration.update(). In all failure cases, there was an update requested that was presumably not intentional. I'm going to try one more push with printfs in the PeriodicUpdater to see if it does indeed receive idle-daily from the fake specialpowers vs from the standard source.
Blocks: 1130101
Attachment #8600967 - Flags: review?(amarchesini) → review+
Depends on: 1162787
See Also: → 1158530
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.