Bug 186360

<rdar://problem/34297947>

Created attachment 342071 [details] Patch

Comment on attachment 342071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342071&action=review > Source/WebCore/dom/NodeRareData.cpp:41 > struct SameSizeAsNodeRareData { > unsigned m_bitfields : 20; > - void* m_pointer[3]; > + void* m_pointer[4]; > }; > > COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall); It looks like Ryosuke at one point believed it was important to performance for this class to stay small: <https://trac.webkit.org/changeset/137003/webkit>. Are we sure it's OK to get bigger, especially just for hardening? I believe that only elements can have inline event listeners. Maybe we reduce the pain by putting the weak pointer factory on element?

(In reply to Geoffrey Garen from comment #3) > Comment on attachment 342071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342071&action=review > > > Source/WebCore/dom/NodeRareData.cpp:41 > > struct SameSizeAsNodeRareData { > > unsigned m_bitfields : 20; > > - void* m_pointer[3]; > > + void* m_pointer[4]; > > }; > > > > COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall); > > It looks like Ryosuke at one point believed it was important to performance > for this class to stay small: > <https://trac.webkit.org/changeset/137003/webkit>. I will let Ryosuke comment on this. > > Are we sure it's OK to get bigger, especially just for hardening? Well, hardening against a very valid crash (see radar). > > I believe that only elements can have inline event listeners. Maybe we > reduce the pain by putting the weak pointer factory on element? In this class, m_originalNode can either be an Element or a Document, so I don't think I can move it to Element / ElementRareData. Note that until fairly recently, we had a WeakPtrFactory in ElementRareData. I killed recently because there were no more users for it. That said, we never saw a regression when I added this WeakPtrFactory in ElementRareData in the past.

> > Are we sure it's OK to get bigger, especially just for hardening? > > Well, hardening against a very valid crash (see radar). Yeah, I guess it's a distraction to question the value of hardening. Let's focus on whether it's OK to grow this data structure, and what alternatives to consider if it's not OK.

Comment on attachment 342071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342071&action=review >>> Source/WebCore/dom/NodeRareData.cpp:41 >>> COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall); >> >> It looks like Ryosuke at one point believed it was important to performance for this class to stay small: <https://trac.webkit.org/changeset/137003/webkit>. >> >> Are we sure it's OK to get bigger, especially just for hardening? >> >> I believe that only elements can have inline event listeners. Maybe we reduce the pain by putting the weak pointer factory on element? > > I will let Ryosuke comment on this. I don't think this is okay. We make zillions of Text nodes with whitespaces in any given page. We should limit this to only Element and also run Speedometer, PLT, & our internal memory tests to ensure there is no regression.

(In reply to Ryosuke Niwa from comment #6) > Comment on attachment 342071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342071&action=review > > >>> Source/WebCore/dom/NodeRareData.cpp:41 > >>> COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall); > >> > >> It looks like Ryosuke at one point believed it was important to performance for this class to stay small: <https://trac.webkit.org/changeset/137003/webkit>. > >> > >> Are we sure it's OK to get bigger, especially just for hardening? > >> > >> I believe that only elements can have inline event listeners. Maybe we reduce the pain by putting the weak pointer factory on element? > > > > I will let Ryosuke comment on this. > > I don't think this is okay. We make zillions of Text nodes with whitespaces > in any given page. > We should limit this to only Element and also run Speedometer, PLT, & our > internal memory tests to ensure there is no regression. As I previously explained, I did not limit this to Element because m_originalNode is a ContainerNode (can be an Element or a Document).

WONTFIX for now, even though this seems power-neutral on Speedometer. I'll try and get better repro-steps for the crash to fix the underlying issue instead.

Mass move bugs into the DOM component.