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

RESOLVED FIXED in Firefox 44

Status

Testing
web-platform-tests
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: noemi, Assigned: Away for a while)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
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")"
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → ehsan
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
Created attachment 8678618 [details] [diff] [review]
Part 1: Ensure that the state of all ServiceWorker instances is up to date when dispatching statechange events
Attachment #8678618 - Flags: review?(bkelly)
(Assignee)

Comment 5

2 years ago
Created attachment 8678619 [details] [diff] [review]
Part 2: Make synced-state.https.html pass
Attachment #8678619 - Flags: review?(bkelly)

Comment 6

2 years ago
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()
Attachment #8678618 - Flags: review?(bkelly) → review+

Updated

2 years ago
Attachment #8678619 - Flags: review?(bkelly) → review+
(Assignee)

Comment 7

2 years ago
(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

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0e600058af6
https://hg.mozilla.org/mozilla-central/rev/fb0887080d19
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.