Bug 206724

Summary: Nullptr crash when setting custom properties on detached style
Product: WebKit Reporter: Sunny He <sunny_he>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Sunny He 2020-01-23 17:56:48 PST
rdar://56687964
Comment 1 Sunny He 2020-01-23 18:07:37 PST
Created attachment 388631 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-01-26 12:06:37 PST
Comment on attachment 388631 [details]
Patch

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

> LayoutTests/fast/dom/StyleSheet/detached-style-set-custom-property.html:18
> +    <script src="../../../resources/js-test-post.js"></script>

Please use ../../../resources/js-test.js instead of js-test-pre.js, then js-test-post.js is not needed.
Comment 3 Darin Adler 2020-01-26 21:33:17 PST
Comment on attachment 388631 [details]
Patch

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

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:260
> +        ASSERT(document);

Why is this assertion safe? Since m_lastDocument is a WeakPtr, what guarantees it won’t be null?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:63
> +    WeakPtr<Document> m_lastDocument;

This doesn’t seem like the right approach to me.
Comment 4 Darin Adler 2020-01-26 21:38:12 PST
Comment on attachment 388631 [details]
Patch

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

What causes the crash? It seems like the setCustomProperty function already tries to handle a null document. Where does the crash occur?

>> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:63
>> +    WeakPtr<Document> m_lastDocument;
> 
> This doesn’t seem like the right approach to me.

I think this could be done with just a plain Ref<Document> that is initialized when the declaration is created and always used. I don’t think there’s any real risk of a reference cycle. And then we would not have to complicate the clearParentRule and clearParentElement functions.

But more importantly, I don’t understand what about a null document causes a crash.
Comment 5 Ryosuke Niwa 2020-01-27 16:18:39 PST
(In reply to Darin Adler from comment #4)
> Comment on attachment 388631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388631&action=review
> 
> What causes the crash? It seems like the setCustomProperty function already
> tries to handle a null document. Where does the crash occur?

Ah, the crash happens because we try to dereference parentStyleSheet(), not document itself.

> >> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:63
> >> +    WeakPtr<Document> m_lastDocument;
> > 
> > This doesn’t seem like the right approach to me.
> 
> I think this could be done with just a plain Ref<Document> that is
> initialized when the declaration is created and always used. I don’t think
> there’s any real risk of a reference cycle. And then we would not have to
> complicate the clearParentRule and clearParentElement functions.

We can't quite do that the document associated with a given PropertySetCSSStyleDeclaration can change when the owner element gets adopted to another document (e.g. via document.adoptNode).
Comment 6 Ryosuke Niwa 2020-01-27 16:21:36 PST
It's a good point that we can probably use a RefPtr<Document> instead of WeakPtr<Document> although we should make sure we're not introducing new leaks by running CSS layout tests with --world-leaks.
Comment 7 Sunny He 2020-01-27 16:33:27 PST
Comment on attachment 388631 [details]
Patch

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

>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:260
>> +        ASSERT(document);
> 
> Why is this assertion safe? Since m_lastDocument is a WeakPtr, what guarantees it won’t be null?

This assert shouldn't be here, it's a leftover from debugging. setCustomProperty can handle null document, the crash was from assuming there would always be a parentStyleSheet() when there isn't a parentElement().

>> LayoutTests/fast/dom/StyleSheet/detached-style-set-custom-property.html:18
>> +    <script src="../../../resources/js-test-post.js"></script>
> 
> Please use ../../../resources/js-test.js instead of js-test-pre.js, then js-test-post.js is not needed.

Thanks, will fix.
Comment 8 Darin Adler 2020-01-27 16:50:09 PST
Comment on attachment 388631 [details]
Patch

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

>>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:260
>>> +        ASSERT(document);
>> 
>> Why is this assertion safe? Since m_lastDocument is a WeakPtr, what guarantees it won’t be null?
> 
> This assert shouldn't be here, it's a leftover from debugging. setCustomProperty can handle null document, the crash was from assuming there would always be a parentStyleSheet() when there isn't a parentElement().

If parentStyleSheet is null too, then why can't we let document be null? Why do we need this m_lastDocument trick?
Comment 9 Ryosuke Niwa 2020-01-27 16:51:36 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 388631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388631&action=review
> 
> >>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:260
> >>> +        ASSERT(document);
> >> 
> >> Why is this assertion safe? Since m_lastDocument is a WeakPtr, what guarantees it won’t be null?
> > 
> > This assert shouldn't be here, it's a leftover from debugging. setCustomProperty can handle null document, the crash was from assuming there would always be a parentStyleSheet() when there isn't a parentElement().
> 
> If parentStyleSheet is null too, then why can't we let document be null? Why
> do we need this m_lastDocument trick?

If document is null, then we wouldn't be able to get to the custom property registry & parse the property as "*" as opposed to whatever syntax currently defined by the author script. On the other hand, that's not a terrible outcome for now since custom registry isn't really something we currently ship.
Comment 10 Sunny He 2020-01-27 18:44:36 PST
Created attachment 388958 [details]
Patch
Comment 11 WebKit Commit Bot 2020-01-29 00:04:25 PST
The commit-queue encountered the following flaky tests while processing attachment 388958 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-01-29 00:05:02 PST
Comment on attachment 388958 [details]
Patch

Clearing flags on attachment: 388958

Committed r255340: <https://trac.webkit.org/changeset/255340>
Comment 13 WebKit Commit Bot 2020-01-29 00:05:04 PST
All reviewed patches have been landed.  Closing bug.