Bug 79333 – REGRESSION: Assertion failure in Document::setCompatibilityMode() (!m_styleSheets->length()) in fast/dynamic/crash-paint-no-documentElement-renderer.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

<rdar://problem/10598598>

To reproduce: DumpRenderTree LayoutTests/fast/dynamic/crash-paint-no-documentElement-renderer.html

Result:
ASSERTION FAILED: !m_styleSheets->length()
/Projects/Safari/OpenSource/Source/WebCore/dom/Document.cpp(699) : void WebCore::Document::setCompatibilityMode(WebCore::Document::CompatibilityMode)
1   0x1026f3f45 WebCore::Document::setCompatibilityMode(WebCore::Document::CompatibilityMode)
2   0x102b7b370 WebCore::HTMLTreeBuilder::defaultForInitial()
3   0x102b7603f WebCore::HTMLTreeBuilder::processEndOfFile(WebCore::AtomicHTMLToken&)
4   0x102b71ff0 WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&)
5   0x102b7135b WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&)
6   0x102b7126c WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&)
7   0x102abd39c WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
8   0x102abce55 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
9   0x102abccf4 WebCore::HTMLDocumentParser::prepareToStopParsing()
10  0x102abdf13 WebCore::HTMLDocumentParser::attemptToEnd()
11  0x102abdf68 WebCore::HTMLDocumentParser::finish()
12  0x1026fbc64 WebCore::Document::explicitClose()
13  0x1026f6eea WebCore::Document::close()
14  0x102ea53e5 WebCore::jsHTMLDocumentPrototypeFunctionClose(JSC::ExecState*)
[…]

This is tracked by bug 68859.

Created attachment 128406 [details]
Disable the test that triggers bug 68859

[Alexey Proskuryakov]

(In reply to comment #1)
> This is tracked by bug 68859.

Also by bug 51793 and bug 75135, apparently. Could you please dupe as appropriate?

[Julien Chaffraix]

Comment on attachment 128406 [details]
Disable the test that triggers bug 68859

I don't think this is the way to go. This test doesn't cover bug 68859 (it does hit the ASSERT and I don't have a good solution for it), it covers bug 64284 which is a crash in release. I had disabled the test in debug or skipped it for all platforms to avoid hitting the ASSERT. I don't know what happened but you should add the test to your platform accordingly instead of losing coverage on all platforms.

(In reply to comment #4)
> (From update of attachment 128406 [details])
> I had disabled the test in debug or skipped it for all platforms to avoid hitting the ASSERT

Where and how did you do that?

> I don't know what happened but you should add the test to your platform accordingly instead of losing coverage on all platforms.

Are there platforms that don’t fail the assertion when running this test? Skipped lists are platform-specific and are meant to address platform-specific issues. As far as I can tell, this test would cause the assertion to fail on any platform, so it should be disabled.

[Julien Chaffraix]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 128406 [details] [details])
> > I had disabled the test in debug or skipped it for all platforms to avoid hitting the ASSERT
> 
> Where and how did you do that?

By adding the test to the appropriate test_expectations.txt & Skipped files.

> 
> > I don't know what happened but you should add the test to your platform accordingly instead of losing coverage on all platforms.
> 
> Are there platforms that don’t fail the assertion when running this test? 

No, all platform should hit the assertion in debug and this is expected.

> Skipped lists are platform-specific and are meant to address platform-specific issues. As far as I can tell, this test would cause the assertion to fail on any platform, so it should be disabled.

I understand your point and agree that it is unfortunate to have this test hit the assertion on all platforms. If the test were providing no coverage and be a maintenance burden, then yes disabling would be the way to go. However here it checks against regressing the crash from bug 64284 in release so the situation is not clear cut to me and I would rather keep this coverage.

I don't have the context of your wanting to disable this test so maybe there is something I am missing here.

[Sam Weinig]

Comment on attachment 128406 [details]
Disable the test that triggers bug 68859

The correct thing to do here is disable the test until the assertion can be dealt with.

[Sam Weinig]

> I don't have the context of your wanting to disable this test so maybe there is something I am missing here.

The context is that it is not acceptable to have a test that asserts.

[Julien Chaffraix]

(In reply to comment #8)
> > I don't have the context of your wanting to disable this test so maybe there is something I am missing here.
> 
> The context is that it is not acceptable to have a test that asserts.

So be it, though you should at least remove the test from the test_expectations.txt & Skipped files (I wonder if this will break platforms that have it). Also adding a comment to bug 68859 that this test should be re-enabled once we fix the assert.

(In reply to comment #9)
> (In reply to comment #8)
> > > I don't have the context of your wanting to disable this test so maybe there is something I am missing here.
> > 
> > The context is that it is not acceptable to have a test that asserts.
> 
> So be it, though you should at least remove the test from the test_expectations.txt & Skipped files

Done.

> Also adding a comment to bug 68859 that this test should be re-enabled once we fix the assert.

Will do.

Landed in <http://trac.webkit.org/r108725>.

[Alexey Proskuryakov]

> Also by bug 51793 and bug 75135, apparently. Could you please dupe as appropriate?

Could someone who understands this bug please clean up the duplicates?

[Julien Chaffraix]

(In reply to comment #11)
> > Also by bug 51793 and bug 75135, apparently. Could you please dupe as appropriate?
> 
> Could someone who understands this bug please clean up the duplicates?

Done, those were indeed duplicates.

Thanks Mitz for addressing the comments.