Bug 223495 – BrandedStructure should keep its members alive.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

rdar://75565765

[Mark Lam]

Created attachment 423682 [details]
proposed patch.

[Mark Lam]

Created attachment 423684 [details]
proposed patch.

[Saam Barati]

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 423689 [details]
patch for landing.

[Caio Lima]

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.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 423689 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Mark Lam]

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.

[Mark Lam]

Landed in r274727: <http://trac.webkit.org/r274727>.

[Caio Lima]

(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.