Bug 227789 – Remove more custom binding code with GenerateAddOpaqueRoot

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Ryosuke Niwa]

Now that we have added GenerateAddOpaqueRoot, use in more IDL files and get rid of custom bindings code.

[Ryosuke Niwa]

Created attachment 433114 [details]
Patch

[Chris Dumez]

Comment on attachment 433114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433114&action=review

> Source/WebCore/dom/AbortController.h:43
> +    void* opaqueRoot() const { return m_signal.ptr(); }

Can't we use `GenerateAddOpaqueRoot=signal` in the IDL to avoid having to add a new opaqueRoot() function? I think we may need to add a WTF::getPtr() call in the generated bindings if missing to convert the reference into a pointer but otherwise, I don't see why it wouldn't work?

Same comment may apply to other classes in this patch.

> Source/WebCore/html/track/TrackListBase.cpp:70
> +    return m_element.get()->opaqueRoot();

why .get()? Doesn't seem needed.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 433114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433114&action=review
> 
> > Source/WebCore/dom/AbortController.h:43
> > +    void* opaqueRoot() const { return m_signal.ptr(); }
> 
> Can't we use `GenerateAddOpaqueRoot=signal` in the IDL to avoid having to
> add a new opaqueRoot() function? I think we may need to add a WTF::getPtr()
> call in the generated bindings if missing to convert the reference into a
> pointer but otherwise, I don't see why it wouldn't work?
> 
> Same comment may apply to other classes in this patch.

I guess we could do that although I'm somewhat worried that people might make signal() function not thread safe in the future.

> > Source/WebCore/html/track/TrackListBase.cpp:70
> > +    return m_element.get()->opaqueRoot();
> 
> why .get()? Doesn't seem needed.

m_element->opaqueRoot() will assert because of the thread safety check in WeakPtr.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 433114 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433114&action=review
> > 
> > > Source/WebCore/dom/AbortController.h:43
> > > +    void* opaqueRoot() const { return m_signal.ptr(); }
> > 
> > Can't we use `GenerateAddOpaqueRoot=signal` in the IDL to avoid having to
> > add a new opaqueRoot() function? I think we may need to add a WTF::getPtr()
> > call in the generated bindings if missing to convert the reference into a
> > pointer but otherwise, I don't see why it wouldn't work?
> > 
> > Same comment may apply to other classes in this patch.
> 
> I guess we could do that although I'm somewhat worried that people might
> make signal() function not thread safe in the future.
> 
> > > Source/WebCore/html/track/TrackListBase.cpp:70
> > > +    return m_element.get()->opaqueRoot();
> > 
> > why .get()? Doesn't seem needed.
> 
> m_element->opaqueRoot() will assert because of the thread safety check in
> WeakPtr.

Seems like either this is unsafe or we should allow operator->() to get called from the GC thread?
I am still a bit unclear on what safe to do on the GC thread.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> (In reply to Ryosuke Niwa from comment #3)
> > m_element->opaqueRoot() will assert because of the thread safety check in
> > WeakPtr.
> 
> Seems like either this is unsafe or we should allow operator->() to get
> called from the GC thread?
> I am still a bit unclear on what safe to do on the GC thread.

Maybe? But the fact this is explicit is a good thing. We don't want people to start doing "interesting" things in GC threads.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Ryosuke Niwa from comment #3)
> > > m_element->opaqueRoot() will assert because of the thread safety check in
> > > WeakPtr.
> > 
> > Seems like either this is unsafe or we should allow operator->() to get
> > called from the GC thread?
> > I am still a bit unclear on what safe to do on the GC thread.
> 
> Maybe? But the fact this is explicit is a good thing. We don't want people
> to start doing "interesting" things in GC threads.

I thought that dereferencing a WeakPtr on the GC thread was unsafe, which is why it has a threading assertion. Your code is working around that by calling get() but then still dereferencing.

In my book, you are already doing an "interesting" thing on the GC thread and I don't personally understand why it is safe (although it may well be).

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > (In reply to Ryosuke Niwa from comment #3)
> > > > m_element->opaqueRoot() will assert because of the thread safety check in
> > > > WeakPtr.
> > > 
> > > Seems like either this is unsafe or we should allow operator->() to get
> > > called from the GC thread?
> > > I am still a bit unclear on what safe to do on the GC thread.
> > 
> > Maybe? But the fact this is explicit is a good thing. We don't want people
> > to start doing "interesting" things in GC threads.
> 
> I thought that dereferencing a WeakPtr on the GC thread was unsafe, which is
> why it has a threading assertion. Your code is working around that by
> calling get() but then still dereferencing.
> 
> In my book, you are already doing an "interesting" thing on the GC thread
> and I don't personally understand why it is safe (although it may well be).

BTW, the previous code was calling `root(wrapped().element())` which looked safer to me at least.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Ryosuke Niwa from comment #5)
> > > (In reply to Chris Dumez from comment #4)
> > > > (In reply to Ryosuke Niwa from comment #3)
> > > > > m_element->opaqueRoot() will assert because of the thread safety check in
> > > > > WeakPtr.
> > > > 
> > > > Seems like either this is unsafe or we should allow operator->() to get
> > > > called from the GC thread?
> > > > I am still a bit unclear on what safe to do on the GC thread.
> > > 
> > > Maybe? But the fact this is explicit is a good thing. We don't want people
> > > to start doing "interesting" things in GC threads.
> > 
> > I thought that dereferencing a WeakPtr on the GC thread was unsafe, which is
> > why it has a threading assertion. Your code is working around that by
> > calling get() but then still dereferencing.
> > 
> > In my book, you are already doing an "interesting" thing on the GC thread
> > and I don't personally understand why it is safe (although it may well be).
> 
> BTW, the previous code was calling `root(wrapped().element())` which looked
> safer to me at least.

In JSNodeCustom.h, we see that:

inline void* root(Node* node)
{
    return node ? node->opaqueRoot() : nullptr;
}

inline void* root(Node& node)
{
    return root(&node);
}

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > (In reply to Ryosuke Niwa from comment #3)
> > > > m_element->opaqueRoot() will assert because of the thread safety check in
> > > > WeakPtr.
> > > 
> > > Seems like either this is unsafe or we should allow operator->() to get
> > > called from the GC thread?
> > > I am still a bit unclear on what safe to do on the GC thread.
> > 
> > Maybe? But the fact this is explicit is a good thing. We don't want people
> > to start doing "interesting" things in GC threads.
> 
> I thought that dereferencing a WeakPtr on the GC thread was unsafe, which is
> why it has a threading assertion. Your code is working around that by
> calling get() but then still dereferencing.
> 
> In my book, you are already doing an "interesting" thing on the GC thread
> and I don't personally understand why it is safe (although it may well be).

To be honest, none of these are safe. Our DOM GC implementation is pretty badly broken. It's somewhat shocking we haven't gotten a lot of crashes from it so far.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Chris Dumez from comment #6)
> > > (In reply to Ryosuke Niwa from comment #5)
> > > > (In reply to Chris Dumez from comment #4)
> > > > > (In reply to Ryosuke Niwa from comment #3)
> > > > > > m_element->opaqueRoot() will assert because of the thread safety check in
> > > > > > WeakPtr.
> > > > > 
> > > > > Seems like either this is unsafe or we should allow operator->() to get
> > > > > called from the GC thread?
> > > > > I am still a bit unclear on what safe to do on the GC thread.
> > > > 
> > > > Maybe? But the fact this is explicit is a good thing. We don't want people
> > > > to start doing "interesting" things in GC threads.
> > > 
> > > I thought that dereferencing a WeakPtr on the GC thread was unsafe, which is
> > > why it has a threading assertion. Your code is working around that by
> > > calling get() but then still dereferencing.
> > > 
> > > In my book, you are already doing an "interesting" thing on the GC thread
> > > and I don't personally understand why it is safe (although it may well be).
> > 
> > BTW, the previous code was calling `root(wrapped().element())` which looked
> > safer to me at least.
> 
> In JSNodeCustom.h, we see that:
> 
> inline void* root(Node* node)
> {
>     return node ? node->opaqueRoot() : nullptr;
> }
> 
> inline void* root(Node& node)
> {
>     return root(&node);
> }

I see. I didn't remember that code. I completely agree you code is not less safe than it used to be then. I still think it would look much nicer if you used GenerateAddOpaqueRoot=foo instead of introducing new opaqueRoot() functions.

However, if you disagree, feel free to land as is, I still think this is a good improvement.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #10)
>
> I see. I didn't remember that code. I completely agree you code is not less
> safe than it used to be then. I still think it would look much nicer if you
> used GenerateAddOpaqueRoot=foo instead of introducing new opaqueRoot()
> functions.
> 
> However, if you disagree, feel free to land as is, I still think this is a
> good improvement.

Oh sure, I'm making that change. It looks like the patch never got uploaded? Will land with that change.

[Ryosuke Niwa]

Created attachment 433166 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 433166 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Ryosuke Niwa]

Created attachment 433167 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 433170 [details]
Patch for landing

[EWS]

Committed r279761 (239532@main): <https://commits.webkit.org/239532@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433170 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80348006>