Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

Forked from bug 1293277 to add a ref-counted WorkerHolderToken.
Blocks: 1413606
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df82219658dc
Add a ref-counted WorkerHolderToken() class. r=baku
Comment on attachment 8924213 [details] [diff] [review]
Add a ref-counted WorkerHolderToken() class. r=baku

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

note: I realize this already landed, so confusingly setting the feedback flag.

::: dom/workers/WorkerHolderToken.cpp
@@ +95,5 @@
> +  if (aStatus < mShutdownStatus || mShuttingDown) {
> +    return true;
> +  }
> +
> +  mShuttingDown = true;

Should this come after the loop to deal with the case that AddListener() is called from one of the WorkerShuttingDown() listeners?  (Because otherwise, WorkerShuttingDown() will be invoked twice for the newly added Listener.)

@@ +99,5 @@
> +  mShuttingDown = true;
> +
> +  // Start the asynchronous destruction of our actors.  These will call back
> +  // into RemoveActor() once the actor is destroyed.
> +  for (uint32_t i = 0; i < mListenerList.Length(); ++i) {

This iteration idiom seems like it will skip listeners if they remove themselves from the WorkerShuttingDown call.  Should mListenerList be an nsTObserverArray and use its iterators?
Attachment #8924213 - Flags: feedback?(bkelly)
I can add a follow-on patch to perform the loop operation from an array copy.
I think an array copy still might result in weird semantics unless you check if the listener you're about to invoke on is still present in the canonical underlying array, because otherwise you might invoke on an already removed-listener.  (In such a case, the current location of mShuttingDown would be correct, because freshly added callbacks wouldn't be invoked by the traversal of the copy, which maybe was your intent.)
Ok, I'll do a follow-up to use nsTObserverArray.
Attachment #8924213 - Flags: feedback?(bkelly)
Attachment #8924276 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3cea75ce1d
P2 Make WorkerHolderToken use an nsTObserverArray to allow for mutation during iteration. r=asuth
https://hg.mozilla.org/mozilla-central/rev/df82219658dc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reopen until the follow-up lands tomorrow.  Sorry, for this.  I was hoping I was in time for the merge.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/2a3cea75ce1d
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.