Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051993002:
Drop unecessary use of CrossThreadPersistent by CanvasAsyncBlobCreator. (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. |
DescriptionDrop unecessary use of CrossThreadPersistent by CanvasAsyncBlobCreator.
The callback object that the CanvasAsyncBlobCreator passes along to a
background thread can be kept as a simple Member<>; no need to
involve CrossThreadPersistent<> and risk inadvertently introducing
leaks.
R=
BUG=
Committed: https://crrev.com/089aa85a72d06184c60104b1c1c6f84d1e2eb334
Cr-Commit-Position: refs/heads/master@{#399181}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 24 (9 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. Something the GC plugin ought to have called out (..it soon will.)
On 2016/06/09 21:08:33, sof wrote: > please take a look. > > Something the GC plugin ought to have called out (..it soon will.) Thanks, but keishi@ is planning to use CrossThreadPersistent for all cross-origin references when he enables the per-thread heap... We should add a check to the GC plugin to check that CrossThreadPersistent is not traced, but we need to allow CrossThreadPersistent in on-heap classes.
On 2016/06/09 23:37:03, haraken wrote: > On 2016/06/09 21:08:33, sof wrote: > > please take a look. > > > > Something the GC plugin ought to have called out (..it soon will.) > > Thanks, but keishi@ is planning to use CrossThreadPersistent for all > cross-origin references when he enables the per-thread heap... > > We should add a check to the GC plugin to check that CrossThreadPersistent is > not traced, but we need to allow CrossThreadPersistent in on-heap classes. Please read the code & how the callback is used.
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/2051993002/1
The CQ bit was unchecked by sigbjornf@opera.com
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/2051993002/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/2051993002/1
The CQ bit was unchecked by sigbjornf@opera.com
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/2051993002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Drop unecessary use of CrossThreadPersistent by CanvasAsyncBlobCreator. The callback object that the CanvasAsyncBlobCreator passes along to a background thread can be kept as a simple Member<>; no need to involve CrossThreadPersistent<> and risk inadvertently introducing leaks. R= BUG= ========== to ========== Drop unecessary use of CrossThreadPersistent by CanvasAsyncBlobCreator. The callback object that the CanvasAsyncBlobCreator passes along to a background thread can be kept as a simple Member<>; no need to involve CrossThreadPersistent<> and risk inadvertently introducing leaks. R= BUG= Committed: https://crrev.com/089aa85a72d06184c60104b1c1c6f84d1e2eb334 Cr-Commit-Position: refs/heads/master@{#399181} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/089aa85a72d06184c60104b1c1c6f84d1e2eb334 Cr-Commit-Position: refs/heads/master@{#399181}
Message was sent while issue was closed.
https://codereview.chromium.org/2051993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp (left): https://codereview.chromium.org/2051993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:259: // Since toBlob is done, timeout events are no longer needed. So we clear This CL may have introduced some leaks https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b... Taking a look (not clearing the callback the reason..?)
Message was sent while issue was closed.
On 2016/06/13 06:15:27, sof wrote: > https://codereview.chromium.org/2051993002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp > (left): > > https://codereview.chromium.org/2051993002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:259: // > Since toBlob is done, timeout events are no longer needed. So we clear > This CL may have introduced some leaks > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b... > > Taking a look (not clearing the callback the reason..?) Idle tasks posted by CanvasAsyncBlobCreator to handle the encoding creates closures holding CrossThreadPersistent<>s. These aren't scheduled before the main thread gives up and takes over, hence the idle task closures will retain CanvasAsyncBlobCreator objects (=> the blob callback is also retained & it holds onto a script environment => reported leak.) We can clear the callback upon completion, but these idle task closures shouldn't keep a strong persistent.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2064123002/ by kjellander@chromium.org. The reason for reverting is: Breaks WebKit Linux Leak bot: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b... unexpected_failures: virtual/threaded/fast/canvas-toBlob/canvas-toBlob-file-vs-blob.html virtual/display_list_2d_canvas/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html virtual/threaded/fast/canvas-toBlob/canvas-toBlob-invalid.html fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html virtual/gpu/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html virtual/threaded/fast/canvas-toBlob/canvas-toBlob-defaultpng.html virtual/gpu/fast/canvas/canvas-toBlob-jpeg-medium-quality.html . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
