Bug 217846 - Add AbstractRange
Summary: Add AbstractRange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
: 183561 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-10-16 15:03 PDT by Darin Adler
Modified: 2021-09-02 02:11 PDT (History)
17 users (show)

See Also:


Attachments
Patch (33.96 KB, patch)
2020-10-16 15:20 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (40.46 KB, patch)
2020-10-16 17:04 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (40.46 KB, patch)
2020-10-16 17:09 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (42.04 KB, patch)
2020-10-16 17:36 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (42.88 KB, patch)
2020-10-16 19:22 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-10-16 15:03:48 PDT
Add AbstractRange
Comment 1 Darin Adler 2020-10-16 15:07:09 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.
Comment 2 Darin Adler 2020-10-16 15:20:52 PDT
Created attachment 411623 [details]
Patch
Comment 3 Ryosuke Niwa 2020-10-16 15:40:38 PDT
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 4 Darin Adler 2020-10-16 16:19:02 PDT
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.
Comment 5 Ryosuke Niwa 2020-10-16 16:35:46 PDT
(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.
Comment 6 Darin Adler 2020-10-16 17:04:57 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2020-10-16 17:09:24 PDT
Created attachment 411633 [details]
Patch
Comment 8 Ryosuke Niwa 2020-10-16 17:12:37 PDT
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.
Comment 9 Darin Adler 2020-10-16 17:36:58 PDT
Created attachment 411635 [details]
Patch
Comment 10 Darin Adler 2020-10-16 19:16:54 PDT
Anyone understand why JSAbstractRange.h seems to not be generated on GTK/WPE?
Comment 11 Darin Adler 2020-10-16 19:17:44 PDT
I thought that adding AbstractRange.idl to CMakeLists.txt would be good enough.
Comment 12 Darin Adler 2020-10-16 19:18:22 PDT
Same failure on all CMake platforms, it seems.
Comment 13 Darin Adler 2020-10-16 19:19:14 PDT
I think I figured it out, needed to edit Headers.cmake.
Comment 14 Darin Adler 2020-10-16 19:22:17 PDT
Created attachment 411644 [details]
Patch
Comment 15 Darin Adler 2020-10-17 09:41:39 PDT
Does anyone understand the meaning of the iOS-wk2 EWS failure?
Comment 16 Darin Adler 2020-10-17 13:33:59 PDT
Committed r268648: <https://trac.webkit.org/changeset/268648>
Comment 17 Radar WebKit Bug Importer 2020-10-17 13:34:20 PDT
<rdar://problem/70410623>
Comment 18 Manuel Rego Casasnovas 2021-09-02 02:11:31 PDT
*** Bug 183561 has been marked as a duplicate of this bug. ***