Closed
Bug 1280507
Opened 8 years ago
Closed 8 years ago
Simplify context loss handler
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
Attachments
(1 file)
Currently it uses manual addref/releases. At the very least it shouldn't do this.
Assignee | ||
Comment 1•8 years ago
|
||
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/
Attachment #8763127 -
Flags: review?(jmuizelaar)
Comment 2•8 years ago
|
||
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?
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8763127 -
Attachment is obsolete: true
Attachment #8763127 -
Flags: review?(jmuizelaar)
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
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/
Attachment #8763127 -
Attachment is obsolete: false
Attachment #8763127 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 6•8 years ago
|
||
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.
Flags: needinfo?(jmuizelaar)
Updated•8 years ago
|
Attachment #8763127 -
Flags: review?(jmuizelaar) → review-
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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/
Attachment #8763127 -
Flags: review- → review?(jmuizelaar)
Assignee | ||
Comment 9•8 years ago
|
||
Green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff729cec6505
Comment 10•8 years ago
|
||
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()
Attachment #8763127 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 12•8 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b419a38b9c9 Simplify context loss handler. - r=jrmuizel
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Pushed by jgilbert@mozilla.com: 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/846095c02369 https://hg.mozilla.org/mozilla-central/rev/98cacda13404
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•