Now that we have added GenerateAddOpaqueRoot, use in more IDL files and get rid of custom bindings code.
Created attachment 433114 [details] Patch
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.
(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.
(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.
(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.
(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).
(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 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); }
(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.
(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.
(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.
Created attachment 433166 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 433166 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 433167 [details] Patch for landing
Created attachment 433170 [details] Patch for landing
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].
<rdar://problem/80348006>