Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2060153002:
Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. (Closed)
|
Created:
4 years, 6 months ago by sof Modified:
4 years, 6 months ago CC:
chromium-reviews, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, Justin Novosad, dglazkov+blink, Rik, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHave CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'.
If image encoding should be attempted done via idle tasks,
CanvasAsyncBlobCreator schedules an idle task along with a delayed
task on the main thread to check if the idle task has been scheduled
before too long. If not, the delayed task will handle the encoding
instead (still on the main thread.)
The idle tasks represent opportunistic work, and should not keep the
CanvasAsyncBlobCreator alive until they eventually do get to run.
Consequently, make them keep a weak 'this' reference.
This addresses leaks exposed by r399181.
R=
BUG=
NOTRY=true
Committed: https://crrev.com/e60314435575d80cd8cd74196fdf8356f69adb92
Cr-Commit-Position: refs/heads/master@{#399445}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 20 (10 generated)
Description was changed from ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. R= BUG= ========== to ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= ==========
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. Follow up on https://codereview.chromium.org/2051993002/
https://codereview.chromium.org/2060153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp (left): https://codereview.chromium.org/2060153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:259: } I'm fine with also promptly clearing m_callback here upon completion, if that's considered preferable. Not doing so has the benefit of exposing leaks; CanvasAsyncBlobCreator ought to be no longer strongly referenced at this point.
LGTM
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2060153002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2060153002/1
win_chromium_rel_ng is not having the best of days. https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel... completed successfully, but bot completion of the recipe_results step hasn't been detected. Hence it will time out eventually; 3rd time (un)lucky with this bot for this particular CL.. NOTRYing.
The CQ bit was unchecked by sigbjornf@opera.com
Description was changed from ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= ========== to ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= NOTRY=true ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2060153002/1
Message was sent while issue was closed.
Description was changed from ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= NOTRY=true ========== to ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= NOTRY=true ========== to ========== Have CanvasAsyncBlobCreator speculative idle tasks keep a weak 'this'. If image encoding should be attempted done via idle tasks, CanvasAsyncBlobCreator schedules an idle task along with a delayed task on the main thread to check if the idle task has been scheduled before too long. If not, the delayed task will handle the encoding instead (still on the main thread.) The idle tasks represent opportunistic work, and should not keep the CanvasAsyncBlobCreator alive until they eventually do get to run. Consequently, make them keep a weak 'this' reference. This addresses leaks exposed by r399181. R= BUG= NOTRY=true Committed: https://crrev.com/e60314435575d80cd8cd74196fdf8356f69adb92 Cr-Commit-Position: refs/heads/master@{#399445} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e60314435575d80cd8cd74196fdf8356f69adb92 Cr-Commit-Position: refs/heads/master@{#399445} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
