Bug 206724 – Nullptr crash when setting custom properties on detached style

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sunny He]

rdar://56687964

[Sunny He]

Created attachment 388631 [details]
Patch

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Sunny He]

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.

[Darin Adler]

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?

[Ryosuke Niwa]

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

[Sunny He]

Created attachment 388958 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388958 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388958 [details]
Patch

Clearing flags on attachment: 388958

Committed r255340: <https://trac.webkit.org/changeset/255340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.