Closed
Bug 1189685
Opened 9 years ago
Closed 9 years ago
"Harness status: ERROR" when running "synced-state.https.html" test
Categories
(Testing :: web-platform-tests, defect)
Testing
web-platform-tests
Tracking
(firefox42 affected, firefox44 fixed)
RESOLVED
FIXED
mozilla44
People
(Reporter: noemi, Assigned: ehsan.akhgari)
References
Details
Attachments
(2 files)
4.37 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
3.04 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Attachment #8678618 -
Flags: review?(bkelly)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8678619 -
Flags: review?(bkelly)
Comment 6•9 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•9 years ago
|
Attachment #8678619 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 7•9 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e600058af6 https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0887080d19
Comment 9•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0e600058af6 https://hg.mozilla.org/mozilla-central/rev/fb0887080d19
Status: NEW → RESOLVED
Closed: 9 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.
Description
•