1231974 - Intermittent TEST-UNEXPECTED-FAIL | unregister-then-register-new-script.https.html | Registering a new script URL while an unregistered registration is in use - assert_unreached: assert_equals the new worker should control a new document

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This test depends on GC timing do no longer control the previous frame.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fix wpt GC timing dependency by explicitly navigating a frame to stop being a controlled client. r=ehsan

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

We need to examine this test case for correctness and determine what the spec should do here.  There is already bug 1227007 to do that.

I'm just going to disable this test for now.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fix unstable unregister-then-register-new-script wpt tests for now. r=ehsan

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/283333d47b1b

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8697602 [details] [diff] [review]
Fix wpt GC timing dependency by explicitly navigating a frame to stop being a controlled client. r=ehsan

A debug try to see what is going on here.  I don't think my GC theory above is accurate:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f35207eecb0

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/283333d47b1b

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

We're getting an update executed for the scope using a different registration object which then does not see the script URL has changed.  This then re-activates the old script URL.

Its still unclear why we are getting this new registration.  It could be related to a necko channel failure that appears to be happening in the ServiceWorkerScriptCache layer for some reason.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I think we're essentially hitting the same race described in:

  https://github.com/slightlyoff/ServiceWorker/issues/783

We should really look up the registrations in the job Start() instead of getting our references from outside of the queue synchronization mechanism.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Remove unneeded ServiceWorkerScriptJobBase abstract class. r=ehsan

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Store scope on ServiceWorkerJobBase. r=ehsan

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Store the principal on the ServiceWorkerJobBase. r=ehsan

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Lazy load registration and verify it does not change in service worker jobs. r=ehsan

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Re-enable unregister-then-register-new-script.https.html. r=ehsan

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a79efc8703

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Lazy load registration and verify it does not change in service worker jobs. r=ehsan

Updated to fix a failure path that called Done() synchronously from a job Start().

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

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Lazy load registration and verify it does not change in service worker jobs. r=ehsan

Try to fix that same failure case again.  This time with better logic.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eee01cc54bf

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

There are still a couple intermittents I want to investigate, but I think these patches should land.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Abort update-triggered install jobs if the service worker script has changed. r=ehsan

So, there is still a race in the spec even after making these agreed changes:

  https://github.com/slightlyoff/ServiceWorker/issues/783#issuecomment-164813284

This patch allows us to abort the install job if it was triggered from an update and the newest worker script has changed on us.

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Try is looking pretty good.  Large number of retriggers have completed with no service worker wpt failures.

This should be good to go once review completes.

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8698281 [details] [diff] [review]
P4 Lazy load registration and verify it does not change in service worker jobs. r=ehsan

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +970,5 @@
>  
> +  nsresult
> +  EnsureRegistration()
> +  {
> +    AssertIsOnMainThread();

what about if mRegistration already exist?

@@ +974,5 @@
> +    AssertIsOnMainThread();
> +
> +    RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    if (NS_WARN_IF(!swm)) {
> +      mRegistration = nullptr;

this should not be needed.

@@ +1164,5 @@
>  
>    void
>    ContinueAfterInstallEvent(bool aInstallEventSuccess)
>    {
> +    nsresult rv = EnsureRegistration();

are we sure we don't have a mRegistration here?

@@ +1340,5 @@
>                     const nsACString& aMaxScope) override
>    {
>      RefPtr<ServiceWorkerRegisterJob> kungFuDeathGrip = this;
> +
> +    nsresult rv = EnsureRegistration();

move it after mCanceled check

@@ +1492,5 @@
>    {
>      AssertIsOnMainThread();
>      RefPtr<ServiceWorkerRegisterJob> kungFuDeathGrip = this;
> +
> +    nsresult rv = EnsureRegistration();

move this after the if(mCanceled) check.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #20)
> > +  nsresult
> > +  EnsureRegistration()
> > +  {
> > +    AssertIsOnMainThread();
> 
> what about if mRegistration already exist?

This is allowed by design.  But we verify that the current registration for this scope is not a new object and clear mRegistration if it has changed.

> @@ +974,5 @@
> > +    AssertIsOnMainThread();
> > +
> > +    RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> > +    if (NS_WARN_IF(!swm)) {
> > +      mRegistration = nullptr;
> 
> this should not be needed.

Why?  I want to ensure as an invariant that if EnsureRegistration returns a failure code, then mRegistration is nullptr.  If shutdown happens in the middle of the job, then I think SWM can be nullptr, but still have a mRegistration set.

> >    void
> >    ContinueAfterInstallEvent(bool aInstallEventSuccess)
> >    {
> > +    nsresult rv = EnsureRegistration();
> 
> are we sure we don't have a mRegistration here?

We probably do.  This EnsureRegistration() call is verifying it has not changed on us.  (For example, something didn't clear the registration and create a new one.)

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

I'll change the name of the method to EnsureAndVerifyRegistration() to clarify its doing more than setting mRegistration.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Lazy load registration and verify it does not change in service worker jobs. r=baku

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b4e90792d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8589c8a2cfb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8bcaa0c789
https://hg.mozilla.org/integration/mozilla-inbound/rev/01f41e2e28fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b10997a06c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a595c83b21f

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9b4e90792d2
https://hg.mozilla.org/mozilla-central/rev/8589c8a2cfb5
https://hg.mozilla.org/mozilla-central/rev/cc8bcaa0c789
https://hg.mozilla.org/mozilla-central/rev/01f41e2e28fb
https://hg.mozilla.org/mozilla-central/rev/1b10997a06c6
https://hg.mozilla.org/mozilla-central/rev/0a595c83b21f