Summary: | ASSERT(!m_styleSheets->length()) can be easily hit when playing with document.open | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | DOM | Assignee: | Julien Chaffraix <jchaffraix> | ||||||
Status: | NEW --- | ||||||||
Severity: | Minor | CC: | abarth, ap, cdumez, eric, koivisto, mitz, opendarwin, simon.fraser, zhenghao | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 64284 | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2011-09-26 18:18:32 PDT
Created attachment 108774 [details]
Proposed removal.
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. 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. 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). Comment on attachment 108774 [details]
Proposed removal.
Clearing the review flag while I find another way to handle the ASSERT.
Renaming the bug as the consensus is that the ASSERT should be kept and my assumptions were wrong. Created attachment 110024 [details]
Tweaked the ASSERT to not trigger in this case as it is too fragile. Added a comment about the fix.
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>. *** Bug 51793 has been marked as a duplicate of this bug. *** *** Bug 75135 has been marked as a duplicate of this bug. *** *** Bug 89441 has been marked as a duplicate of this bug. *** 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). |