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

Status: RESOLVED DUPLICATE of bug 1334383

(Core :: XPCOM, defect)

Description

[Treeherder Bug Filer]

Filed by: kgupta [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=90274339&repo=graphics

https://queue.taskcluster.net/v1/task/bvWMmdHtT3u6XtnAUs1EOw/runs/0/artifacts/public/logs/live_backing.log

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1355315: Don't complain if nsTimerEvent is destroyed on the TimerThread. Allows some stuff to be simplified.

Review commit: https://reviewboard.mozilla.org/r/136566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136566/

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8864912 [details]
Bug 1355315: Don't complain if nsTimerEvent is destroyed on the TimerThread. Allows some stuff to be simplified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136566/diff/1-2/