1280507 - Simplify context loss handler

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Currently it uses manual addref/releases.
At the very least it shouldn't do this.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1280507 - Simplify context loss handler. -

Use a self-referential RefPtr instead of manual AddRef/Release.
Reuse DisableTimer for when a worker is dead.

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

Comment 2

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/59458/#review56942

Why not just cancel the timer on the destruction of WebGLContextLossHandler instead of trying to have the timer keep the WebGLContextLossHandler alive?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> https://reviewboard.mozilla.org/r/59458/#review56942
> 
> Why not just cancel the timer on the destruction of WebGLContextLossHandler
> instead of trying to have the timer keep the WebGLContextLossHandler alive?

Canceling can race with the timer occurring and triggering the callback.
Also these patches are out of date, sorry!

I have a much better solution that is nearly working.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > https://reviewboard.mozilla.org/r/59458/#review56942
> > 
> > Why not just cancel the timer on the destruction of WebGLContextLossHandler
> > instead of trying to have the timer keep the WebGLContextLossHandler alive?
> 
> Canceling can race with the timer occurring and triggering the callback.
> Also these patches are out of date, sorry!

I don't think this is the case. The timer runs on the same thread and we check whether it's been canceled when firing. This idiom is also used elsewhere.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8763127 [details]
Bug 1280507 - Simplify context loss handler. -

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

Comment 6

[Kelsey Gilbert [:jgilbert]]

Alright, that should do it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a1010644236&selectedJob=22754206

Current patch fixes the implicit constructor complaint from the static analysis jobs.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8763127 [details]
Bug 1280507 - Simplify context loss handler. -

https://reviewboard.mozilla.org/r/59458/#review57284

::: dom/canvas/WebGLContextLossHandler.h:35
(Diff revision 2)
>  
> +    friend class WatchdogTimerEvent;
> +
>  public:
> -    NS_INLINE_DECL_REFCOUNTING(WebGLContextLossHandler)
> +    // We need THEADSAFE because we might be cc'd by the cc thread.
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebGLContextLossHandler)

There is no cycle collection thread. We need to find out what's going on here.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8763127 [details]
Bug 1280507 - Simplify context loss handler. -

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

Comment 9

[Kelsey Gilbert [:jgilbert]]

Green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff729cec6505

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8763127 [details]
Bug 1280507 - Simplify context loss handler. -

https://reviewboard.mozilla.org/r/59458/#review57446

This looks like a healthy change :)

::: dom/canvas/WebGLContextLossHandler.cpp:31
(Diff revision 3)
> -        new ContextLossWorkerRunnable(eventRef);
> -    return mEventTarget->Dispatch(wrappedEvent, aFlags);
> -}
>  
> -NS_IMETHODIMP
> -ContextLossWorkerEventTarget::DelayedDispatch(already_AddRefed<nsIRunnable>,
> +    NS_IMETHOD Notify(nsITimer*) override {
> +        if (mHandler) {

It can be worth writing:
RefPtr<WebGLContextLossHandler> handler = mHandler;
if (handler)
    handler->TimerCallback();

This avoids the caller having to ensure that handler stays alive for duration of TimerCallback()

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b419a38b9c9
Simplify context loss handler. - r=jrmuizel

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8c537dc68e88eeb7d75edd519bcf6f219f65de for breaking offscreencanvas on Windows (mostly Win7 opt and Win8 opt and debug, in a https://treeherder.mozilla.org/logviewer.html#?job_id=30754362&repo=mozilla-inbound sort of way).

About time to disable all the tests on every platform until it's actually something that can sometimes run without crashing, wouldn't you say?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/846095c02369
Simplify context loss handler. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/98cacda13404
Disable OffscreenCanvas tests.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/846095c02369
https://hg.mozilla.org/mozilla-central/rev/98cacda13404