Bug 217846 – Add AbstractRange

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Add AbstractRange

[Darin Adler]

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.

[Darin Adler]

Created attachment 411623 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Darin Adler]

Created attachment 411632 [details]
Patch

[Darin Adler]

Created attachment 411633 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

Created attachment 411635 [details]
Patch

[Darin Adler]

Anyone understand why JSAbstractRange.h seems to not be generated on GTK/WPE?

[Darin Adler]

I thought that adding AbstractRange.idl to CMakeLists.txt would be good enough.

[Darin Adler]

Same failure on all CMake platforms, it seems.

[Darin Adler]

I think I figured it out, needed to edit Headers.cmake.

[Darin Adler]

Created attachment 411644 [details]
Patch

[Darin Adler]

Does anyone understand the meaning of the iOS-wk2 EWS failure?

[Darin Adler]

Committed r268648: <https://trac.webkit.org/changeset/268648>

[Radar WebKit Bug Importer]

<rdar://problem/70410623>

[Manuel Rego Casasnovas]

*** Bug 183561 has been marked as a duplicate of this bug. ***