...
Created attachment 402190 [details] Patch
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.
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.
Created attachment 402216 [details] Patch
Moved m_isManipulated to NodeRareData and reduced size of m_connectedFrameCount.
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.
(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.
(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.
(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?
Created attachment 402511 [details] Patch
Uploaded a new patch using m_manipulatedNodes and removing node from set when node is destroyed.
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.
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.
Created attachment 402873 [details] Patch for landing
Committed r263563: <https://trac.webkit.org/changeset/263563> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402873 [details].
<rdar://problem/64809758>
<rdar://problem/64809761>