@ r248804, the Mojave Leaks bot showed 401 unique leaks https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4031 @ r248807, it showed 3519 unique leaks https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4032
<rdar://problem/54536113>
<rdar://problem/54536121>
Looking at the revisions in the range, this looks like a change that could be responsible: Don't use union to store NodeRareData* and RenderObject* https://trac.webkit.org/changeset/248807/webkit
I can't get the leaks viewer to show any data, it hangs at loading 26/66 files https://build.webkit.org/LeaksViewer/?url=%2Fresults%2FApple%20Mojave%20%28Leaks%29%2Fr248807%20%284032%29%2F I see a bunch of 503 errors in the console, but clicking the links for those resources works.
Created attachment 377181 [details] Fixes the bug
Comment on attachment 377181 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review r=me > Source/WebCore/dom/NodeRareData.h:277 > + virtual ~NodeRareData() > + { } Do we need to declare this inline? I recall saving space by not inlining destructors of some classes. (I guess this is rare, though? :)
Comment on attachment 377181 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review >> Source/WebCore/dom/NodeRareData.h:277 >> + { } > > Do we need to declare this inline? I recall saving space by not inlining destructors of some classes. (I guess this is rare, though? :) Otherwise ElementRareData::~ElementRareData would make an actual function call, which would be silly. We used to not inline virtual functions to make compilation faster but I don't think it matters for this file.
Committed r249076: <https://trac.webkit.org/changeset/249076>
Comment on attachment 377181 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review > Source/WebCore/ChangeLog:9 > + NodeRareData didn't have a virtual destructor. As a result, member variables > + of ElementRareData did not get destructed properly. Usually the complier gives an error/warning if a class is a base class and it does not have a virtual destructor. I wonder what was the reason for having the complier silent in the case of NodeRareData.
(In reply to Said Abou-Hallawa from comment #9) > Comment on attachment 377181 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377181&action=review > > > Source/WebCore/ChangeLog:9 > > + NodeRareData didn't have a virtual destructor. As a result, member variables > > + of ElementRareData did not get destructed properly. > > Usually the complier gives an error/warning if a class is a base class and > it does not have a virtual destructor. I wonder what was the reason for > having the complier silent in the case of NodeRareData. There is no virtual function in ElementRareData. I wish clang did warn about it too. Also confirmed that this fixed most of the leak: https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4132
We shouldn't virtualize a performance sensitive type like this just to be able to destroy it (blot from unnecessary pointer, extra virtual function call, another vtable to corrupt). Please revert this patch and implement destruction manually like it was done before.
Comment on attachment 377181 [details] Fixes the bug I don't think it makes sense to mark this patch as r- since it has already been landed.
(In reply to Antti Koivisto from comment #11) > We shouldn't virtualize a performance sensitive type like this just to be > able to destroy it (blot from unnecessary pointer, extra virtual function > call, another vtable to corrupt). I'm not sure about that. vtable pointer in NodeRareData shouldn't matter because we'd create it for a far fewer DOM nodes after r248807. > Please revert this patch and implement destruction manually like it was done before. I've thought about this approach and in fact wrote such a patch locally but decided not to do that because: 1. We won't be able to use unique_ptr or rather defeats the point of it if we had to leakPtr and delete it manually. 2. Relying on Node::m_flags to decide which destructor to use can result in a type confusion if Node::m_flags was heap overrun.
I think a good alternative is what Saam suggested which is to override deleter, and we can use a single bit of m_connectedFrameCount to record whether it's NodeRareData or not. This will avoid (1) as well as (2). The flag in NodeRareData being heap overran is a moot point since such an overrun can also override vtable pointer.
> I've thought about this approach and in fact wrote such a patch locally but > decided not to do that because: > 1. We won't be able to use unique_ptr or rather defeats the point of it if > we had to leakPtr and delete it manually. I don't think the aesthetic concern is worth an otherwise unnecessary vptr in this particular code (it may be in other places). > 2. Relying on Node::m_flags to decide which destructor to use can result in > a type confusion if Node::m_flags was heap overrun. Is vptr somehow more secure? This pattern has been used here for a long time.
(In reply to Antti Koivisto from comment #15) > > I've thought about this approach and in fact wrote such a patch locally but > > decided not to do that because: > > 1. We won't be able to use unique_ptr or rather defeats the point of it if > > we had to leakPtr and delete it manually. > > I don't think the aesthetic concern is worth an otherwise unnecessary vptr > in this particular code (it may be in other places). It's not an aesthetic concern. It's easy to introduce bugs when exotic memory management mechanisms are used. > > 2. Relying on Node::m_flags to decide which destructor to use can result in > > a type confusion if Node::m_flags was heap overrun. > > Is vptr somehow more secure? This pattern has been used here for a long time. Yes. With vtable pointer, only memory corruption of NodeRareData itself would result in type confusion whereas the old code would have let Node::m_nodeFlags to cause a type confusion in NodeRareData itself. Since the whole point of r248807 is not let that kind of type confusion happen (which had been a source of number of security bugs), going back to the old model isn't desirable.
Let's go with the deleter approach.
(In reply to Ryosuke Niwa from comment #14) > I think a good alternative is what Saam suggested which is to override > deleter, and we can use a single bit of m_connectedFrameCount to record > whether it's NodeRareData or not. > > This will avoid (1) as well as (2). The flag in NodeRareData being heap > overran is a moot point since such an overrun can also override vtable > pointer. That seems like a good plan. Clearly my review of r248807 was too hasty, it shouldn't have been r+'d in the first place considering this obvious issue. Sorry about that.
(In reply to Antti Koivisto from comment #18) > (In reply to Ryosuke Niwa from comment #14) > > I think a good alternative is what Saam suggested which is to override > > deleter, and we can use a single bit of m_connectedFrameCount to record > > whether it's NodeRareData or not. > > > > This will avoid (1) as well as (2). The flag in NodeRareData being heap > > overran is a moot point since such an overrun can also override vtable > > pointer. > > That seems like a good plan. > > Clearly my review of r248807 was too hasty, it shouldn't have been r+'d in > the first place considering this obvious issue. Sorry about that. Wanna use a new bug to override deleter? I was thinking that I can just re-open this bug and post a new patch.
Re-opening is good.
Reopening.
Ugh... I have to fix makeUnique because it doesn't know how to take Deleter or default_delete. Probably have to do that on Monday.
Created attachment 377423 [details] Address Antti's comment
Comment on attachment 377423 [details] Address Antti's comment This is a good way to do this.
Comment on attachment 377423 [details] Address Antti's comment Clearing flags on attachment: 377423 Committed r249198: <https://trac.webkit.org/changeset/249198>
All reviewed patches have been landed. Closing bug.