Bug 213333 – Text manipulation should observe adjacent elements with new renderer together

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Sihui Liu]

...

[Sihui Liu]

Created attachment 402190 [details]
Patch

[Wenson Hsieh]

Comment on attachment 402190 [details]
Patch

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

> Source/WebCore/dom/Node.h:675
> +    bool m_isManipulated { false };

This overall approach seems reasonable to me, but we should avoid making Node larger (dump-class-layout on WebCore::Node with release  shows 72 bytes with no padding). We should probably put it in NodeRareData instead.

[Darin Adler]

Comment on attachment 402190 [details]
Patch

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

>> Source/WebCore/dom/Node.h:675
>> +    bool m_isManipulated { false };
> 
> This overall approach seems reasonable to me, but we should avoid making Node larger (dump-class-layout on WebCore::Node with release  shows 72 bytes with no padding). We should probably put it in NodeRareData instead.

That’s right. We must not add this bool to Node.

[Sihui Liu]

Created attachment 402216 [details]
Patch

[Sihui Liu]

Moved m_isManipulated to NodeRareData and reduced size of m_connectedFrameCount.

[Ryosuke Niwa]

Comment on attachment 402216 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        To fix this issue completely, this patch adds a property isManipulated to NodeRareData. Now 
> +        TextManipulationController can know the state of all types of Nodes and thus can skip the manipulated Nodes 
> +        during observation.

Hm... using a bit in NodeRareData isn't great because
allocating NodeRareData would consume quite a bit of memory for each element.
Why not just replace m_manipulatedElements with m_manipulatedNodes?

Since we never de-reference objects in m_manipulatedElements, it's okay to store a stale pointer.
We could also make it void* or some kind of a new smart pointer type that disallows dereferencing if we wanted.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #6)
> Hm... using a bit in NodeRareData isn't great because
> allocating NodeRareData would consume quite a bit of memory for each element.
> Why not just replace m_manipulatedElements with m_manipulatedNodes?

That sounds smart.

> Since we never de-reference objects in m_manipulatedElements, it's okay to
> store a stale pointer.

I don’t think that’s right.

As far as I know, there is *nothing* safe you can do with pointers to deleted objects, even if they are not dereferenced, except maybe comparing to null. For example, comparing a pointer to a deleted object with a pointer to another object could give an incorrect result. A stale pointer can happen to be equal to a pointer to a new object that is allocated re-using the same memory.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> (In reply to Ryosuke Niwa from comment #6)
> > Since we never de-reference objects in m_manipulatedElements, it's okay to
> > store a stale pointer.
> 
> I don’t think that’s right.
> 
> As far as I know, there is *nothing* safe you can do with pointers to
> deleted objects, even if they are not dereferenced, except maybe comparing
> to null. For example, comparing a pointer to a deleted object with a pointer
> to another object could give an incorrect result. A stale pointer can happen
> to be equal to a pointer to a new object that is allocated re-using the same
> memory.

Sure, but that's still safe in this case. The worst thing that could happen is that we'd mistakenly think the content had already been manipulated and fail to notify the client. It's still safe in the sense of security guarantee although the program may still misbehave (by not reacting to newly inserted content) if we had such a bug.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #8)
> It's still safe in the sense of security
> guarantee although the program may still misbehave (by not reacting to newly
> inserted content) if we had such a bug.

OK, but could we use something like WeakPtr and have predictable behavior instead?

[Sihui Liu]

Created attachment 402511 [details]
Patch

[Sihui Liu]

Uploaded a new patch using m_manipulatedNodes and removing node from set when node is destroyed.

[Geoffrey Garen]

Comment on attachment 402511 [details]
Patch

r=me

I slightly prefer the rare data solution, since I like it when you don't have to worry about doing interesting pointer safety things in your destructor. Also, you only pay the memory cost when you use the feature, which won't happen on most pages.

That said, I think this approach works, and resolves all review comments so far. We can always discuss the rare data approach more when we have a chance, and reconsider it.

[Ryosuke Niwa]

I think the long term solution is to add more bit flags directly on Node and/or make it possible to create a WeakPtr to Node. I have vague plans for both.

[Sihui Liu]

Created attachment 402873 [details]
Patch for landing

[EWS]

Committed r263563: <https://trac.webkit.org/changeset/263563>

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

[Radar WebKit Bug Importer]

<rdar://problem/64809758>

[Radar WebKit Bug Importer]

<rdar://problem/64809761>