Summary: | Add AbstractRange | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | annevk, annulen, benjamin, calvaris, cdumez, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, kondapallykalyan, megan_gardner, rniwa, ryuan.choi, sam, sergio, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Darin Adler
2020-10-16 15:03:48 PDT
Megan Gardner pointed out that we are likely to need AbstractRange to implement proposed CSS Custom Highlight API https://drafts.csswg.org/css-highlight-api-1 and convinced me this is OK for WebKit. At first I was worried about overhead, but the cost is only the additional size of a virtual table pointer in Range and StaticRange objects, plus a little more code when converting a StaticRange to a SimpleRange due to multiple inheritance. In this first patch, there is some unused code, since we are not yet using AbstractRange as the type of anything in IDL files, but it still should all be ready to go. Created attachment 411623 [details]
Patch
Comment on attachment 411623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411623&action=review r=me with the iso-heap changes. > Source/WebCore/dom/AbstractRange.idl:32 > + Exposed=Window, Should we add a runtime flag for this? > Source/WebCore/dom/Range.h:41 > -class Range : public RefCounted<Range> { > +class Range final : public AbstractRange { Please make this iso-heaped. > Source/WebCore/dom/Range.h:46 > + Node& startContainer() const final { return m_start.container(); } It's kind of silly that Range can't use SimpleRange as its internal data structure. Then one of these member functions need to be virtual. > Source/WebCore/dom/StaticRange.h:35 > +class StaticRange final : public AbstractRange, public SimpleRange { Please make this iso-heaped. > Source/WebCore/dom/StaticRange.idl:28 > + JSGenerateToNativeObject, It occurs to me that StaticRange also needs custom is reachable function. InputEvent and HighlightRangeGroup hold onto them but we don't keep this wrapper alive. Comment on attachment 411623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411623&action=review Looks like there are some WPT progressions from this change. >> Source/WebCore/dom/AbstractRange.idl:32 >> + Exposed=Window, > > Should we add a runtime flag for this? I don’t think we should. This is a super-low-risk addition. >> Source/WebCore/dom/Range.h:41 >> +class Range final : public AbstractRange { > > Please make this iso-heaped. OK. Will do! Just curious, is that newly important now because of polymorphism or was it already important before? >> Source/WebCore/dom/Range.h:46 >> + Node& startContainer() const final { return m_start.container(); } > > It's kind of silly that Range can't use SimpleRange as its internal data structure. > Then one of these member functions need to be virtual. Interesting idea. It’s not a trivial change to make. Range uses a class RangeBoundaryPoint to share code between start and end and using SimpleRange as a base would require refactoring that. Should be possible, though! >> Source/WebCore/dom/StaticRange.idl:28 >> + JSGenerateToNativeObject, > > It occurs to me that StaticRange also needs custom is reachable function. > InputEvent and HighlightRangeGroup hold onto them but we don't keep this wrapper alive. Good point. I will make a test for this and tackle it. Annoying to have to do it for HighlightRangeGroup. (In reply to Darin Adler from comment #4) > Comment on attachment 411623 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411623&action=review > > Looks like there are some WPT progressions from this change. > > >> Source/WebCore/dom/AbstractRange.idl:32 > >> + Exposed=Window, > > > > Should we add a runtime flag for this? > > I don’t think we should. This is a super-low-risk addition. > > >> Source/WebCore/dom/Range.h:41 > >> +class Range final : public AbstractRange { > > > > Please make this iso-heaped. > > OK. Will do! > > Just curious, is that newly important now because of polymorphism or was it > already important before? I think it would have been always important but now it's even more important since we'd now have vtable pointer. If attacker can override that pointer, they can get to RCE rather quickly. > >> Source/WebCore/dom/Range.h:46 > >> + Node& startContainer() const final { return m_start.container(); } > > > > It's kind of silly that Range can't use SimpleRange as its internal data structure. > > Then one of these member functions need to be virtual. > > Interesting idea. It’s not a trivial change to make. Range uses a class > RangeBoundaryPoint to share code between start and end and using SimpleRange > as a base would require refactoring that. Should be possible, though! Yeah, I know why we don't wanna do it in this patch but it seems a bit silly that we have a completely different data structures to basically store the same stuff. > > >> Source/WebCore/dom/StaticRange.idl:28 > >> + JSGenerateToNativeObject, > > > > It occurs to me that StaticRange also needs custom is reachable function. > > InputEvent and HighlightRangeGroup hold onto them but we don't keep this wrapper alive. > > Good point. I will make a test for this and tackle it. Annoying to have to > do it for HighlightRangeGroup. Yeah. I had pointed out to Megan but we ended up not enabling it by default so didn't matter as much. However, InputEvent is a shipping feature so we most certainly need to fix that sooner than later. Created attachment 411632 [details]
Patch
Created attachment 411633 [details]
Patch
Comment on attachment 411633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411633&action=review > Source/WebCore/dom/Range.h:42 > + WTF_MAKE_ISO_ALLOCATED(Range); You also need WTF_MAKE_ISO_ALLOCATED_IMPL in cpp files. Created attachment 411635 [details]
Patch
Anyone understand why JSAbstractRange.h seems to not be generated on GTK/WPE? I thought that adding AbstractRange.idl to CMakeLists.txt would be good enough. Same failure on all CMake platforms, it seems. I think I figured it out, needed to edit Headers.cmake. Created attachment 411644 [details]
Patch
Does anyone understand the meaning of the iOS-wk2 EWS failure? Committed r268648: <https://trac.webkit.org/changeset/268648> *** Bug 183561 has been marked as a duplicate of this bug. *** |