rdar://75565765
Created attachment 423682 [details] proposed patch.
Created attachment 423684 [details] proposed patch.
Comment on attachment 423684 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423684&action=review > Source/JavaScriptCore/runtime/BrandedStructure.cpp:59 > +void BrandedStructure::destruct() Can we put this in the header and inline it? It’s so trivial
Comment on attachment 423684 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423684&action=review >> Source/JavaScriptCore/runtime/BrandedStructure.cpp:59 >> +void BrandedStructure::destruct() > > Can we put this in the header and inline it? It’s so trivial Will do.
Created attachment 423689 [details] patch for landing.
Comment on attachment 423689 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=423689&action=review > Source/JavaScriptCore/ChangeLog:27 > + 1. m_parentBrand was not visited by visitChildren(). I'm curious to know why this is necessary. I introduced it at first, but then I removed based on the reasoning that `m_parentBrand` will always point backwards to a structure from the transition chain. Given that, when `m_previous` is visited, it will eventually visit its `m_parentBrand`. Did you find a case where this is not true? I took it a bit further and figured out that is possible to implement the brand check without `m_parentBrand`, but decided to keep it there as a shortcut to avoid traverse the entire chain.
Tools/Scripts/svn-apply failed to apply attachment 423689 [details] to trunk. Please resolve the conflicts and upload a new patch.
Comment on attachment 423689 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=423689&action=review >> Source/JavaScriptCore/ChangeLog:27 >> + 1. m_parentBrand was not visited by visitChildren(). > > I'm curious to know why this is necessary. I introduced it at first, but then I removed based on the reasoning that `m_parentBrand` will always point backwards to a structure from the transition chain. Given that, when `m_previous` is visited, it will eventually visit its `m_parentBrand`. Did you find a case where this is not true? > > I took it a bit further and figured out that is possible to implement the brand check without `m_parentBrand`, but decided to keep it there as a shortcut to avoid traverse the entire chain. See Structure::setBrandTransition(), specifically this scenario: ``` if (structure->isDictionary()) { PropertyTable* table = transition->ensurePropertyTable(vm); transition->pin(holdLock(transition->m_lock), vm, table); ``` pin() does the following: ``` void Structure::pin(const AbstractLocker&, VM& vm, PropertyTable* table) { setIsPinnedPropertyTable(true); setPropertyTable(vm, table); clearPreviousID(); m_transitionPropertyName = nullptr; } ``` clearPreviousID() clears the structure chain link, which leaves m_parentBrand vulnerable.
Landed in r274727: <http://trac.webkit.org/r274727>.
(In reply to Mark Lam from comment #8) > Comment on attachment 423689 [details] > patch for landing. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423689&action=review > > >> Source/JavaScriptCore/ChangeLog:27 > >> + 1. m_parentBrand was not visited by visitChildren(). > > > > I'm curious to know why this is necessary. I introduced it at first, but then I removed based on the reasoning that `m_parentBrand` will always point backwards to a structure from the transition chain. Given that, when `m_previous` is visited, it will eventually visit its `m_parentBrand`. Did you find a case where this is not true? > > > > I took it a bit further and figured out that is possible to implement the brand check without `m_parentBrand`, but decided to keep it there as a shortcut to avoid traverse the entire chain. > > See Structure::setBrandTransition(), specifically this scenario: > > ``` > if (structure->isDictionary()) { > PropertyTable* table = transition->ensurePropertyTable(vm); > transition->pin(holdLock(transition->m_lock), vm, table); > ``` > > pin() does the following: > ``` > void Structure::pin(const AbstractLocker&, VM& vm, PropertyTable* table) > { > setIsPinnedPropertyTable(true); > setPropertyTable(vm, table); > clearPreviousID(); > m_transitionPropertyName = nullptr; > } > ``` > > clearPreviousID() clears the structure chain link, which leaves > m_parentBrand vulnerable. Oh, I totally missed that. Thanks for explaining and for the fix as well.