rdar://56687964
Created attachment 388631 [details] Patch
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 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 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.
(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).
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 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 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?
(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.
Created attachment 388958 [details] Patch
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 on attachment 388958 [details] Patch Clearing flags on attachment: 388958 Committed r255340: <https://trac.webkit.org/changeset/255340>
All reviewed patches have been landed. Closing bug.