Bug 198319 – Add write barrier to JSValueInWrappedObject for garbage collection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

As title.

[Sihui Liu]

Created attachment 370818 [details]
Patch

[Sam Weinig]

Comment on attachment 370818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370818&action=review

> Source/WebCore/ChangeLog:9
> +        Add write barrier to JSValueInWrappedObject for garbage collection
> +        https://bugs.webkit.org/show_bug.cgi?id=198319
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * bindings/js/JSValueInWrappedObject.h:
> +        (WebCore::cachedPropertyValue):

What issue is this fixing? Can a regression test be added?

[Ryosuke Niwa]

Comment on attachment 370818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370818&action=review

>> Source/WebCore/ChangeLog:9
>> +        (WebCore::cachedPropertyValue):
> 
> What issue is this fixing? Can a regression test be added?

Without a write barrier, it's possible that "value" can be collected before value can be prematurely collected.
We also have an issue with JSValueInWrappedObject's m_value using Variant without any lock.
Since m_value can be read concurrency in a GC thread, that is also unsafe.

[Filip Pizlo]

Comment on attachment 370818 [details]
Patch

This looks good to me. I don’t expect you to write a test for missing write barriers.

[Darin Adler]

Comment on attachment 370818 [details]
Patch

I’m excited that using a class for this means we can fix this across the entire project with a single patch. In fact it is part of what I intended when I created this class, which tried to standardize an idiom that was spread out inconsistently across a lot of code.

Like Sam, I’d like to know more about how we can tell if we’ve done it right and detect future mistakes. Even if it’s not a something we can make a regression test for, what technique can we use to increase our confidence?

[Sihui Liu]

Created attachment 424042 [details]
Patch for landing

[EWS]

Committed r274890: <https://commits.webkit.org/r274890>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424042 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75749358>

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> Comment on attachment 370818 [details]
> Patch
> 
> I’m excited that using a class for this means we can fix this across the
> entire project with a single patch. In fact it is part of what I intended
> when I created this class, which tried to standardize an idiom that was
> spread out inconsistently across a lot of code.
> 
> Like Sam, I’d like to know more about how we can tell if we’ve done it right
> and detect future mistakes. Even if it’s not a something we can make a
> regression test for, what technique can we use to increase our confidence?

I think this is a tough question. It's akin to using a raw pointer without realizing that an object could have been deleted before the pointer value is cleared. Other than things like ASAN and GuardMalloc, there isn't really a way for us to detect such a bad pointer. I think the situation is similar here and harder because we don't have an equivalent tool. Perhaps, we can do something like what we're trying to do with the smart pointer and come up with some kind of rule we can just follow?