Bug 223495 - BrandedStructure should keep its members alive.
Summary: BrandedStructure should keep its members alive.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-18 19:03 PDT by Mark Lam
Modified: 2021-03-19 11:48 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (4.40 KB, patch)
2021-03-18 19:10 PDT, Mark Lam
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (6.12 KB, patch)
2021-03-18 19:42 PDT, Mark Lam
sbarati: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch for landing. (5.70 KB, patch)
2021-03-18 21:44 PDT, Mark Lam
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-03-18 19:03:55 PDT
rdar://75565765
Comment 1 Mark Lam 2021-03-18 19:10:01 PDT
Created attachment 423682 [details]
proposed patch.
Comment 2 Mark Lam 2021-03-18 19:42:37 PDT
Created attachment 423684 [details]
proposed patch.
Comment 3 Saam Barati 2021-03-18 21:26:31 PDT
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 4 Mark Lam 2021-03-18 21:27:48 PDT
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.
Comment 5 Mark Lam 2021-03-18 21:44:19 PDT
Created attachment 423689 [details]
patch for landing.
Comment 6 Caio Lima 2021-03-19 05:04:07 PDT
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.
Comment 7 EWS 2021-03-19 09:45:50 PDT
Tools/Scripts/svn-apply failed to apply attachment 423689 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 8 Mark Lam 2021-03-19 10:44:50 PDT
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.
Comment 9 Mark Lam 2021-03-19 10:49:55 PDT
Landed in r274727: <http://trac.webkit.org/r274727>.
Comment 10 Caio Lima 2021-03-19 11:48:42 PDT
(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.