Crash on reload when CSS property 'content' has malformed URL.

[James Kozianski]

Crash on reload when CSS property 'content' has malformed URL.

[Alexey Proskuryakov]

I don't like this stack trace, let's move to Security for now.

[Alexey Proskuryakov]

Created attachment 70136 [details]
test case (may crash)

With this test case, I can reliably get an assertion failure with a debug build of ToT. But it doesn't crash Safari 5.0.2.

Were you observing the crash with shipping Safari or Chrome? Could you please attach your test case, or provide a link to a Web site?

#0	0x1029cb73c in WTF::RefCountedBase::derefBase at RefCounted.h:80
#1	0x1028bb261 in WTF::RefCounted<WebCore::StyleImage>::deref at RefCounted.h:138
#2	0x1029a3240 in WebCore::ContentData::deleteContent at ContentData.cpp:66
#3	0x1029a3297 in WebCore::ContentData::clear at ContentData.cpp:32
#4	0x1029a386b in WebCore::ContentData::~ContentData at ContentData.h:45
#5	0x1029a3794 in WTF::deleteOwnedPtr<WebCore::ContentData> at OwnPtrCommon.h:59
#6	0x1029a3854 in WTF::OwnPtr<WebCore::ContentData>::~OwnPtr at OwnPtr.h:57
#7	0x10340ba0a in WebCore::StyleRareNonInheritedData::~StyleRareNonInheritedData at StyleRareNonInheritedData.cpp:104
#8	0x102a77923 in WTF::RefCounted<WebCore::StyleRareNonInheritedData>::deref at RefCounted.h:139
#9	0x1028be6db in WTF::derefIfNotNull<WebCore::StyleRareNonInheritedData> at PassRefPtr.h:58
#10	0x1032e7826 in WTF::RefPtr<WebCore::StyleRareNonInheritedData>::~RefPtr at RefPtr.h:58
#11	0x1032e783d in WebCore::DataRef<WebCore::StyleRareNonInheritedData>::~DataRef at DataRef.h:31
#12	0x1032e4e61 in WebCore::RenderStyle::~RenderStyle at RenderStyle.cpp:161
#13	0x1029cd0af in WTF::RefCounted<WebCore::RenderStyle>::deref at RefCounted.h:139
#14	0x1028c39b3 in WTF::derefIfNotNull<WebCore::RenderStyle> at PassRefPtr.h:58
#15	0x1029cd0f0 in WTF::RefPtr<WebCore::RenderStyle>::~RefPtr at RefPtr.h:58
#16	0x1032e8d09 in WTF::VectorDestructor<true, WTF::RefPtr<WebCore::RenderStyle> >::destruct at Vector.h:90
#17	0x1032e8d37 in WTF::VectorTypeOperations<WTF::RefPtr<WebCore::RenderStyle> >::destruct at Vector.h:245
#18	0x1032e8db5 in WTF::Vector<WTF::RefPtr<WebCore::RenderStyle>, 4ul>::shrink at Vector.h:849
#19	0x1032e8dee in WTF::Vector<WTF::RefPtr<WebCore::RenderStyle>, 4ul>::~Vector at Vector.h:517
#20	0x1032e8e21 in WTF::deleteOwnedPtr<WTF::Vector<WTF::RefPtr<WebCore::RenderStyle>, 4ul> > at OwnPtrCommon.h:59
#21	0x1032e8eba in WTF::OwnPtr<WTF::Vector<WTF::RefPtr<WebCore::RenderStyle>, 4ul> >::~OwnPtr at OwnPtr.h:57
#22	0x1032e4e3a in WebCore::RenderStyle::~RenderStyle at RenderStyle.cpp:161
<...>

[Alexey Proskuryakov]

Actually, it looks like a null ptr crash, I guess we can move it back from security.

[James Kozianski]

Created attachment 70203 [details]
Patch

[James Kozianski]

This particular test case only seems to crash Chromium, possibly because of the differences between GURL and KURL.

There would be a crash if any of the 'return 0;' paths of CSSImageValue::cachedImage() get hit though.

[Alexey Proskuryakov]

What about my test case? It crashed Safari, even if only in debug mode. Please be sure to include a test case that crashes at least on Mac debug buildbot.

[James Kozianski]

Created attachment 70205 [details]
Patch

[James Kozianski]

(In reply to comment #6)
> What about my test case? It crashed Safari, even if only in debug mode. Please be sure to include a test case that crashes at least on Mac debug buildbot.

I've amended my test so that it crashes on both WebKit and Chromium.

[Alexey Proskuryakov]

Thanks for updating the patch!

The patch looks sane. Simon is probably the best reviewer for it, according to svn blame.

[Simon Fraser (smfr)]

Comment on attachment 70205 [details]
Patch

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

r=me but please fix the test.

> LayoutTests/fast/css-generated-content/malformed-url.html:3
> +<div style="content:url(//%);"></div>              <!-- Crashes Chromium -->
> +<div style="content:url(http://|server|/);"></div> <!-- Crashes debug WebKit -->

Please remove the comments, since once this bug is fixed, they will no longer be correct.

I don't like the strategy of navigating to another page to finish the test. Please change this to run the test in an iframe.

[Alexey Proskuryakov]

> I don't like the strategy of navigating to another page to finish the test. Please change this to run the test in an iframe.

Personally, I use these techniques interchangeably. Why is it a bad idea to navigate? It seems that you need two files with either approach, so readability shouldn't be affected. What am I missing?

[Simon Fraser (smfr)]

(In reply to comment #11)
> > I don't like the strategy of navigating to another page to finish the test. Please change this to run the test in an iframe.
> 
> Personally, I use these techniques interchangeably. Why is it a bad idea to navigate? It seems that you need two files with either approach, so readability shouldn't be affected. What am I missing?

Do we have other tests that do this reliably? I'm just a little leery of how well DRT hands navigation during a test.

[Alexey Proskuryakov]

Yes - as long as you call layoutTestController.waitUntilDone()/notifyDone(), it all just works.

[Alexey Proskuryakov]

Some tests also reload of the same document (adding a query or changing window.name to know that it's the second iteration), or navigate to data:<p>PASS<p><script>if (window.layoutTestController) layoutTestController.notifyDone();</script>. That has the benefit of only having one file per test.

[Alexey Proskuryakov]

More precisely, data:text/html,<p>PASS<p><script>if (window.layoutTestController) layoutTestController.notifyDone();</script>

[Simon Fraser (smfr)]

Comment on attachment 70205 [details]
Patch

Based on discussion, the test is fine.

[WebKit Commit Bot]

Comment on attachment 70205 [details]
Patch

Clearing flags on attachment: 70205

Committed r69418: <http://trac.webkit.org/changeset/69418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.