Summary: | BrandedStructure should keep its members alive. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, msaboff, saam, ticaiolima, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mark Lam
2021-03-18 19:03:55 PDT
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. |