Bug 68859 – ASSERT(!m_styleSheets->length()) can be easily hit when playing with document.open

• Component: DOM
• Status: NEW

[Julien Chaffraix]

Kind of a follow up of bug 60935, the ASSERT that was not removed is actually hit on bug 64284. Instead of fixing both issues from the previous bug at the same time, I thought it would be better to provide remove the ASSERT first as it may be controversial.

Patch on his way.

[Julien Chaffraix]

Created attachment 108774 [details]
Proposed removal.

[Alexey Proskuryakov]

Comment on attachment 108774 [details]
Proposed removal.

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

Hmm, but isn't this assertion catching a bug here? Stylesheets should have a matching compatibility mode AFAICT.

> LayoutTests/fast/parser/append-style-followed-by-close.html:7
> +        layoutTestController.waitUntilDone();

No need for waitUntilDone() - onload is called before DRT automatically dumps results.

[Darin Adler]

Comment on attachment 108774 [details]
Proposed removal.

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

> Source/WebCore/dom/Document.cpp:-682
> -    ASSERT(!m_styleSheets->length());

What was the rationale of the original assertion? I assume it’s asserting because if there are style sheets they might have to be processed again to handle the compatibility mode properly. So if we are removing the assertion it seems we might have to add code that does something to cause the style sheets to be processed again.

It’s fairly common to assert something like this to emphasize that there is no work to be done. Work that otherwise we might need to have code to accomplish.

Or maybe the assertion was completely spurious with no value.

We should find out.

[Julien Chaffraix]

Comment on attachment 108774 [details]
Proposed removal.

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

>> LayoutTests/fast/parser/append-style-followed-by-close.html:7
>> +        layoutTestController.waitUntilDone();
> 
> No need for waitUntilDone() - onload is called before DRT automatically dumps results.

I would have agreed with you before writing the test. If you don't put waitUntilDone, you don't dump the alert() on Chromium. As the test is not slow, there seems to be some race-condition (seen only under NRWT not under plain DRT) but I did not investigate it.

>> Source/WebCore/dom/Document.cpp:-682
>> -    ASSERT(!m_styleSheets->length());
> 
> What was the rationale of the original assertion? I assume it’s asserting because if there are style sheets they might have to be processed again to handle the compatibility mode properly. So if we are removing the assertion it seems we might have to add code that does something to cause the style sheets to be processed again.
> 
> It’s fairly common to assert something like this to emphasize that there is no work to be done. Work that otherwise we might need to have code to accomplish.
> 
> Or maybe the assertion was completely spurious with no value.
> 
> We should find out.

The ASSERT was added in bug 44788 - implement HTML5-compliant doctype switching - as a way to catch what any already processed stylesheet. The goal was to underline what you said (that we did not have to reprocess some of the stylesheets).

The default Document's parser will not hit the ASSERT: per HTML5's parsing algorithm, it would have changed the tree builder's state if it sees any <style> element. However any series of document.open, document.addChild, document.close would trigger it. I bet there are other examples if you start mixing up some parsing + document.addChild from JS. Under those assumptions, it seemed fine to remove it. Another way would be to loosen it to skip the check for a JS-created parser (likely with a comment saying that we would need to have a reparsing phase in this case).

[Julien Chaffraix]

Comment on attachment 108774 [details]
Proposed removal.

Clearing the review flag while I find another way to handle the ASSERT.

[Julien Chaffraix]

Renaming the bug as the consensus is that the ASSERT should be kept and my assumptions were wrong.

[Julien Chaffraix]

Created attachment 110024 [details]
Tweaked the ASSERT to not trigger in this case as it is too fragile. Added a comment about the fix.

[Julien Chaffraix]

Comment on attachment 110024 [details]
Tweaked the ASSERT to not trigger in this case as it is too fragile. Added a comment about the fix.

Looking again at the patch, I thought it was blocking some testing for the associated crasher but it's possible to work around the ASSERT by enabling the test only in Release.
Under this light, the patch is not not a good change as we lose coverage for a potentially bad situation. I will just progress with the other bug and use this one for the ASSERT failure.

I disabled a test that was failing this assertion in<http://trac.webkit.org/r108725>.

[Julien Chaffraix]

*** Bug 51793 has been marked as a duplicate of this bug. ***

[Julien Chaffraix]

*** Bug 75135 has been marked as a duplicate of this bug. ***

[Hao Zheng]

*** Bug 89441 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 110024 [details]
Tweaked the ASSERT to not trigger in this case as it is too fragile. Added a comment about the fix.

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

> Source/WebCore/dom/Document.cpp:684
> +    // in ways that are unknown to the parser (like manually inserting stylesheets). FIXME: To solve this issue

This reminded me of this comment: http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L410  Marking the document's style-resolver as needing recalc will cause it to reparse sheets I think.  I suspect clearPageUserSheet, etc. have that effect already (but I have not confirmed).