Closed Bug 1355315 Opened 3 years ago Closed 3 years ago

Intermittent GECKO(3081) | Assertion failure: rc != 0 (destroyed timer off its target thread!), at xpcom/threads/TimerThread.cpp:490

Categories

(Core :: XPCOM, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 1334383

People

(Reporter: intermittent-bug-filer, Assigned: bwc)

References

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file)

Duplicate of this bug: 1355633
Assignee: nobody → docfaraday
Attachment #8864912 - Flags: review?(nfroyd)
Comment on attachment 8864912 [details]
Bug 1355315: Don't complain if nsTimerEvent is destroyed on the TimerThread. Allows some stuff to be simplified.

https://reviewboard.mozilla.org/r/136566/#review140244

This sort of change requires a little more context/explanation than just "it's not a problem anymore".

::: commit-message-57b37:1
(Diff revision 1)
> +Bug 1355315: Don't complain if nsTimerEvent is destroyed on the TimerThread, since that is not a problem anymore. Allows some stuff to be simplified.

This is a very long first line.  Maybe instead:

```
don't complain if nsTimerEvent is destroyed on the timer thread

[justification, further explanation, etc.]
```

Still a long first line, but shorter, at least.

::: xpcom/threads/TimerThread.cpp:177
(Diff revision 1)
> -    return mTimer.forget();
> +    if (!mTarget) {
> +      NS_ERROR("Attempt to post timer event to NULL event target");
> +      return;
> -  }
> +    }

We would just leak before if this happened, but with this change, we'd be racy (and crashy, with release-mode assertions on refcounting), which seems like a less great state of affairs.

::: xpcom/threads/TimerThread.cpp:182
(Diff revision 1)
> -    return mTimer.forget();
> +    if (!mTarget) {
> +      NS_ERROR("Attempt to post timer event to NULL event target");
> +      return;
> -  }
> +    }
>  
> -  void SetTimer(already_AddRefed<nsTimerImpl> aTimer)
> +    mTarget->Dispatch(this, NS_DISPATCH_NORMAL);

Why is this not checked for failure?  Event dispatching is not infallible.

::: xpcom/threads/TimerThread.cpp:670
(Diff revision 1)
> -  RefPtr<nsTimerEvent> event = new nsTimerEvent;
> +  RefPtr<nsTimerEvent> event = new nsTimerEvent(aTimerRef);
>    if (!event) {
> -    return timer.forget();
> +    return;
>    }

Again, we'd leak before if allocation failed for some unfortunate reason, but not we race, which seems worse.
Attachment #8864912 - Flags: review?(nfroyd)
Comment on attachment 8864912 [details]
Bug 1355315: Don't complain if nsTimerEvent is destroyed on the TimerThread. Allows some stuff to be simplified.

https://reviewboard.mozilla.org/r/136566/#review140244

Ok. This is kinda messy. nsTimerImpl, as we know, was _way_ not threadsafe for a long time. nsTimerImpl was also something that the user of the timer held a ref to; it implemented nsITimer. I believe the concern was cases where a thread had stopped accepting new runnables, but was draining its queue, and possibly operating on its ref to the nsTimerImpl. But, since nsTimerImpl is threadsafe (apart from bugs), and since it isn't exposed to the user directly, we probably don't need to worry about this case.

Now, what we maybe should pay attention to, and assert/log, is when TimerThread is holding onto the last ref for _any_ reason. This just shouldn't happen with the code as it is written now, so if it does happen it is a hint that either our understanding of the timer code is seriously flawed, or something has corrupted a refcount or something. There's nothing _fundamentally_ wrong with the TimerThread being the last owner, though.

Thoughts?

> We would just leak before if this happened, but with this change, we'd be racy (and crashy, with release-mode assertions on refcounting), which seems like a less great state of affairs.

Actually, we were not leaking before; that line of code that read like emo poetry ("nsrefcnt rc = timerRef.forget().take()->Release();") does in fact release the ref, just in a very confusing way (the only way to get the resultant refcount).

> Why is this not checked for failure?  Event dispatching is not infallible.

I guess we could log if it fails, but there's nothing else to do.

> Again, we'd leak before if allocation failed for some unfortunate reason, but not we race, which seems worse.

There was no leak before; we would raise red flags in the logging, but that should no longer be a problem.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1334383
Attachment #8864912 - Flags: review?(nfroyd)
You need to log in before you can comment on or make changes to this bug.