1189685 - "Harness status: ERROR" when running "synced-state.https.html" test

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[Noemí Freire (:noemi)]

Test run such as |./mach web-platform-tests _mozilla/service-workers/service-worker/synced-state.https.html|

Actual Result:
Harness status: Error
Error: assert_equals: in activated .active should be activated expected "activated" but got "activating"

Found 1 tests
1 Timeout
* worker objects for the same entity have the same state -> timed out

Traces: https://pastebin.mozilla.org/8838601

Comment 1

[Andrew Overholt [:overholt]]

From jdm's work at https://etherpad.mozilla.org/blink-sw-tests :

"Similar to registration-service-worker-attributes.https.html, the values are just wrong (according to the test, at least), not out of sync."

and for registration-service-worker-attributes.https.html: "This appears to be an event ordering issue where the registration process has advanced to the "activated" state before we run the callback for wait_for_state("installed")"

Comment 2

[(no longer active)]

This bug is caused by the way we dispatch the events in ServiceWorkerInfo::UpdateState().  If the first queued statechange event's handler tries to access a ServiceWorker's mState member in another instance, it will contain the outdated value.

Comment 3

[(no longer active)]

Once that is fixed, there is a race condition in the test.  The [[Activate]] algorithm is invoked from the end of [[Install]], and if the statechange event is dispatched on the main thread after step 8 of [[Activate]] runs, the waiting worker will be set to null, but the test expects to find a valid waiting worker.

Comment 4

[(no longer active)]

Attachment: Part 1: Ensure that the state of all ServiceWorker instances is up to date when dispatching statechange events

Comment 5

[(no longer active)]

Attachment: Part 2: Make synced-state.https.html pass

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8678618 [details] [diff] [review]
Part 1: Ensure that the state of all ServiceWorker instances is up to date when dispatching statechange events

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

r=me with comments

::: dom/workers/ServiceWorkerManager.cpp
@@ +4175,5 @@
> +                     ServiceWorkerState aState)
> +    : mState(aState)
> +  {
> +    for (size_t i = 0; i < aInstances.Length(); ++i) {
> +      mInstances.AppendElement(aInstances[i]);

Can't you just do mInstances(aInstances) in the initialization list?

@@ +4182,5 @@
> +
> +  NS_IMETHODIMP Run()
> +  {
> +    for (size_t i = 0; i < mInstances.Length(); ++i) {
> +      mInstances[i]->SetState(mState);

I think it would be worth a comment that we need to ensure all instances have the same state set atomically before notifying any of them about the change.

@@ +4215,5 @@
>    MOZ_ASSERT_IF(mState == ServiceWorkerState::Activated, aState == ServiceWorkerState::Redundant);
>  #endif
>    mState = aState;
> +  nsCOMPtr<nsIRunnable> r = new ChangeStateUpdater(mInstances, mState);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

r.forget()

Comment 7

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #6)
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +4175,5 @@
> > +                     ServiceWorkerState aState)
> > +    : mState(aState)
> > +  {
> > +    for (size_t i = 0; i < aInstances.Length(); ++i) {
> > +      mInstances.AppendElement(aInstances[i]);
> 
> Can't you just do mInstances(aInstances) in the initialization list?

Unfortunately no, the types are different (mInstances holds RefPtrs here.)

> @@ +4182,5 @@
> > +
> > +  NS_IMETHODIMP Run()
> > +  {
> > +    for (size_t i = 0; i < mInstances.Length(); ++i) {
> > +      mInstances[i]->SetState(mState);
> 
> I think it would be worth a comment that we need to ensure all instances
> have the same state set atomically before notifying any of them about the
> change.

Will do.

> @@ +4215,5 @@
> >    MOZ_ASSERT_IF(mState == ServiceWorkerState::Activated, aState == ServiceWorkerState::Redundant);
> >  #endif
> >    mState = aState;
> > +  nsCOMPtr<nsIRunnable> r = new ChangeStateUpdater(mInstances, mState);
> > +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));
> 
> r.forget()

Sure.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e600058af6
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0887080d19

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/b0e600058af6
https://hg.mozilla.org/mozilla-central/rev/fb0887080d19