Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-registerpaint
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will be used to invoke the paint() function to produce the image, which will be in a (large) subsequent patch.

BUG=578252
Committed: https://crrev.com/ada96b395009c5594c398c6d1d0174fc96a8d78f
Cr-Commit-Position: refs/heads/master@{#384672}

[ikilpatrick, 2016-03-29 03:31:30]

Description was changed from

==========
Implement registerPaint()

BUG=578252
==========

to

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent path.

BUG=578252
==========

[ikilpatrick, 2016-03-29 03:31:30]

ikilpatrick@chromium.org changed reviewers:
+ haraken@chromium.org

[ikilpatrick, 2016-03-29 03:31:50]

Description was changed from

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent path.

BUG=578252
==========

to

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent patch.

BUG=578252
==========

[ikilpatrick, 2016-03-29 03:38:02]

haraken: If there is somebody who has worked on registerElement for custom
element who'd be more appropriate to review the patch, please forward. :)

This tries to match:
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp
But is a little simpler.

I didn't split out the v8 specific code into bindings/modules like custom
elements (i.e.
third_party/WebKit/Source/bindings/core/v8/CustomElementConstructorBuilder.cpp)
as it looks like this was pre-fork?
I can split up into v8 specific classes vs. non if necessary. Just wasn't sure
if this was necessary anymore.

[haraken, 2016-03-29 04:26:15]

https://codereview.chromium.org/1839913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1839913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:30:
ScopedPersistent<v8::Function> m_paint;

Would you elaborate on why these ScopedPersistents don't cause leaks? For
example, isn't it possible that the following cycle is created?

  CSSPaintDefinition ==(ScopedPersistent)=> v8::Function ==(arbitrary
reference)=> WorkletGlobalScope's wrapper ==(Oilpan's Persistent)=>
WorkletGlobalScope ==(Oilpan's Member)=> CSSPaintDefinition

I guess we need to have a way to explicitly break the Oilpan's Member from the
WorkletGlobalScope to the CSSPaintDefinition when the WorkletGlobalScope stops
(or something). Otherwise, WorkletGlobalScope's wrapper won't be collected
forever.

[ikilpatrick, 2016-03-30 19:12:51]

https://codereview.chromium.org/1839913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1839913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:30:
ScopedPersistent<v8::Function> m_paint;
On 2016/03/29 04:26:15, haraken wrote:
> 
> Would you elaborate on why these ScopedPersistents don't cause leaks? For
> example, isn't it possible that the following cycle is created?
> 
>   CSSPaintDefinition ==(ScopedPersistent)=> v8::Function ==(arbitrary
> reference)=> WorkletGlobalScope's wrapper ==(Oilpan's Persistent)=>
> WorkletGlobalScope ==(Oilpan's Member)=> CSSPaintDefinition
> 
> I guess we need to have a way to explicitly break the Oilpan's Member from the
> WorkletGlobalScope to the CSSPaintDefinition when the WorkletGlobalScope stops
> (or something). Otherwise, WorkletGlobalScope's wrapper won't be collected
> forever.
> 

Yup that cycle was being created. Thanks for noticing.

CustomElements stores the definitions on the V8PerContextData object which gets
disposed of before the window in that case. This clears the (strong)
ScopedPersistent references.

I've changed patch to do the same. Create a new Object "CSSPaintBinding" which
has strong ScopedPersistents, this is stored off the V8PerContextData.

The ScopedPersistents in CSSPaintDefinition now have weak ScopedPersistents.

I wrote a test case (PaintWorkletTest.cpp) which tests for the memory leak.
PTAL.

https://codereview.chromium.org/1839913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/1839913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:130:
CSSPaintBindingList m_cssPaintBindings;
Instead of this I could also add a V8PerContextDataForModules which hangs off
this object, but that seemed like overkill?

https://codereview.chromium.org/1839913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.cpp (right):

https://codereview.chromium.org/1839913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.cpp:50: void
WindowPaintWorklet::willDetachGlobalObjectFromFrame()
I'm not sure if the dispose call on the scriptController should be in this or
willDestroyGlobalObjectInFrame.

I believe it should be this as the window's perContextData gets disposed of
here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

This is called from document->detach as few lines above.

Thoughts?

[ikilpatrick, 2016-03-31 01:24:17]

Patchset #7 (id:120001) has been deleted

[haraken, 2016-03-31 02:01:59]

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:32:
ScopedPersistent<v8::Function> m_paint;

What's the point of making CSSPaintDefinition hold these weak persistents? What
are they used for?

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:105:
RefPtrWillBeRawPtr<CSSPaintDefinition> definition =
CSSPaintDefinition::create(scriptController()->getScriptState(), constructor,
paint);

Just to confirm: Once registerPaint is called, you want to keep alive the
|constructor| and |paint| until the worklet dies, right?

If that is the cause, a better idea would be:

1) Introduce WorkletGlobalScope::dispose (see WorkerGlobalScope::dispose:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
WorkletGlobalScope::dispose must be called explicitly when the worklet dies
(i.e., the timing must not depend on any destructor).

2) CSSPaintDefinition can keep strong persistents to |constructor| and |paint|.
Clear the persistents WorkletGlobalScope::dispose.

What do you think?

[ikilpatrick, 2016-03-31 19:34:22]

Patchset #8 (id:160001) has been deleted

[ikilpatrick, 2016-03-31 21:59:05]

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:32:
ScopedPersistent<v8::Function> m_paint;
On 2016/03/31 02:01:59, haraken wrote:
> 
> What's the point of making CSSPaintDefinition hold these weak persistents?
What
> are they used for?

This is the next patch in the pipeline. CSSPaintDefinition will be used for
calling into the paint function to produce an Image.
https://codereview.chromium.org/1834843002/diff/60001/third_party/WebKit/Sour...

I was following CustomElements naming here, might be a better name?

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1839913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:105:
RefPtrWillBeRawPtr<CSSPaintDefinition> definition =
CSSPaintDefinition::create(scriptController()->getScriptState(), constructor,
paint);
On 2016/03/31 02:01:59, haraken wrote:
> 
> Just to confirm: Once registerPaint is called, you want to keep alive the
> |constructor| and |paint| until the worklet dies, right?
> 
> If that is the cause, a better idea would be:
> 
> 1) Introduce WorkletGlobalScope::dispose (see WorkerGlobalScope::dispose:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> WorkletGlobalScope::dispose must be called explicitly when the worklet dies
> (i.e., the timing must not depend on any destructor).
> 
> 2) CSSPaintDefinition can keep strong persistents to |constructor| and
|paint|.
> Clear the persistents WorkletGlobalScope::dispose.
> 
> What do you think?

Yes that's correct. Sounds good. Done.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:76:
V8GCController::collectAllGarbageForTesting(isolate);
Is assuming this OK?

At the moment in PaintWorkletGlobalScope m_paintDefinitions is a:
HeapHashMap<String, Member<CSSPaintDefinition>>

We explicitly need a Heap GC to run (clears the ScopedPersistents in
CSSPaintDefinition), then a V8 GC to run after.

Thoughts?

[haraken, 2016-04-01 00:16:06]

LGTM

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:56:
v8::TryCatch block(isolate);

I don't think you need this TryCatch block.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:63: if
(!constructor->Get(context, v8String(isolate,
"inputProperties")).ToLocal(&inputPropertiesValue)) {

Would you use v8Call macros in V8BindingMacros.h?

I think you can just use v8CallOrCrash(). Get can fail only when workers are
forcibly terminated. It's extremely rare and we don't want to mess up the code
base to handle such extremely rare cases.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:67:
Heap::collectGarbage(BlinkGC::NoHeapPointersOnStack, BlinkGC::GCWithSweep,
BlinkGC::ForcedGC);

Use Heap::collectAllGarbage.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:76:
V8GCController::collectAllGarbageForTesting(isolate);
On 2016/03/31 21:59:05, ikilpatrick wrote:
> Is assuming this OK?
> 
> At the moment in PaintWorkletGlobalScope m_paintDefinitions is a:
> HeapHashMap<String, Member<CSSPaintDefinition>>
> 
> We explicitly need a Heap GC to run (clears the ScopedPersistents in
> CSSPaintDefinition), then a V8 GC to run after.
> 
> Thoughts?

Looks good.

[ikilpatrick, 2016-04-01 03:08:43]

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:56:
v8::TryCatch block(isolate);
On 2016/04/01 00:16:06, haraken wrote:
> 
> I don't think you need this TryCatch block.

Done.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:63: if
(!constructor->Get(context, v8String(isolate,
"inputProperties")).ToLocal(&inputPropertiesValue)) {
On 2016/04/01 00:16:06, haraken wrote:
> 
> Would you use v8Call macros in V8BindingMacros.h?
> 
> I think you can just use v8CallOrCrash(). Get can fail only when workers are
> forcibly terminated. It's extremely rare and we don't want to mess up the code
> base to handle such extremely rare cases.
> 
> 

Get can also fail if the developer does something nasty like:

registerPaint('foo', class {
  get paint() { throw Error('would crash'); }
});

I've switched to v8Call.

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/1839913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:67:
Heap::collectGarbage(BlinkGC::NoHeapPointersOnStack, BlinkGC::GCWithSweep,
BlinkGC::ForcedGC);
On 2016/04/01 00:16:06, haraken wrote:
> 
> Use Heap::collectAllGarbage.

Done.

[haraken, 2016-04-01 03:31:36]

LGTM

[ikilpatrick, 2016-04-01 15:26:33]

On 2016/04/01 03:31:36, haraken wrote:
> LGTM

Thanks haraken!

[ikilpatrick, 2016-04-01 15:26:40]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-04-01 15:26:40]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1839913002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-01 15:26:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839913002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839913002/260001

[commit-bot: I haz the power, 2016-04-01 18:08:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 18:08:04]

Failed to apply patch for
third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h:16
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h' with
conflicts.
  U third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h

Patch:       third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
Index: third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
diff --git a/third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
b/third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
index
c69127a2ad8b2c4a672055544ef1b82c47f85326..469fbef95fc5545728de94499da6920a70f1b3f3
100644
--- a/third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
+++ b/third_party/WebKit/Source/modules/csspaint/WindowPaintWorklet.h
@@ -6,6 +6,7 @@
 #define WindowPaintWorklet_h
 
 #include "core/frame/DOMWindowProperty.h"
+#include "modules/ModulesExport.h"
 #include "platform/Supplementable.h"
 #include "platform/heap/Handle.h"
 
@@ -16,7 +17,7 @@ class ExecutionContext;
 class PaintWorklet;
 class Worklet;
 
-class WindowPaintWorklet final : public
NoBaseWillBeGarbageCollected<WindowPaintWorklet>, public
WillBeHeapSupplement<LocalDOMWindow>, public DOMWindowProperty {
+class MODULES_EXPORT WindowPaintWorklet final : public
NoBaseWillBeGarbageCollected<WindowPaintWorklet>, public
WillBeHeapSupplement<LocalDOMWindow>, public DOMWindowProperty {
     WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(WindowPaintWorklet);
     DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED(WindowPaintWorklet);
     USING_FAST_MALLOC_WILL_BE_REMOVED(WindowPaintWorklet);

[commit-bot: I haz the power, 2016-04-01 18:08:08]

The CQ bit was unchecked by commit-bot@chromium.org

[ikilpatrick, 2016-04-01 18:28:16]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-04-01 18:28:16]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1839913002/#ps280001
(title: "rebase + remove WillBe types.")

[commit-bot: I haz the power, 2016-04-01 18:28:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839913002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839913002/280001

[commit-bot: I haz the power, 2016-04-01 18:41:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 18:41:48]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ikilpatrick, 2016-04-01 19:05:59]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2016-04-01 19:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839913002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839913002/280001

[commit-bot: I haz the power, 2016-04-01 20:09:03]

Description was changed from

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent patch.

BUG=578252
==========

to

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent patch.

BUG=578252
==========

[commit-bot: I haz the power, 2016-04-01 20:09:04]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-04-01 20:10:28]

Description was changed from

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent patch.

BUG=578252
==========

to

==========
Implement PaintWorkletGlobalScope#registerPaint() for the CSS Paint API

The spec needs to be updated on this to more closely match how WebComponents
works, which I'll do later this week.
https://drafts.css-houdini.org/css-paint-api/#dom-paintworkletglobalscope-reg...
(different ordering in spec + doesn't store the paint method in a "definition").

registerPaint stores the result in a CSSPaintDefinition class. This class will
be used to invoke the paint() function to produce the image, which will be in a
(large) subsequent patch.

BUG=578252

Committed: https://crrev.com/ada96b395009c5594c398c6d1d0174fc96a8d78f
Cr-Commit-Position: refs/heads/master@{#384672}
==========

[commit-bot: I haz the power, 2016-04-01 20:10:29]

Patchset 13 (id:??) landed as
https://crrev.com/ada96b395009c5594c398c6d1d0174fc96a8d78f
Cr-Commit-Position: refs/heads/master@{#384672}