Bug 11850 – Webarchive fails to save images referenced in CSS

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Cascading stylesheets allow url() references to images for the following properties:

- background/background-image
- layer-background-image
- list-style/list-style-image

None of these are captured when writing a webarchive for a web page, but should be for faithful reproduction of a saved web page.

[David Kilzer (:ddkilzer)]

Created attachment 25498 [details]
Pre-Patch v1

This patch changes the algorithm used in CSSStyleSheet::addSubresourceURLStrings() from recursive to iterative.

This is the initial step before fixing WebCore to find url() resources in CSS files when saving webarchives.

[David Kilzer (:ddkilzer)]

Comment on attachment 25498 [details]
Pre-Patch v1

(In reply to comment #1)
> Created an attachment (id=25498) [review]
> Pre-Patch v1
> 
> This patch changes the algorithm used in
> CSSStyleSheet::addSubresourceURLStrings() from recursive to iterative.
> 
> This is the initial step before fixing WebCore to find url() resources in CSS
> files when saving webarchives.

Clearing review flag for next patch.

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSheet.cpp
Committed r38787

http://trac.webkit.org/changeset/38787

[David Kilzer (:ddkilzer)]

Created attachment 25753 [details]
Pre-patch #2 v1

Proposed patch to clean up data structures used when collecting URLs of subresources.  This fits much better when adding URLs within stylesheets (or inline styles).

A couple of notes for reviewers:

- The change to WebCore::Node::getSubresourceURLs() removes the parseURL() method from being called on most (if not all) subresource URL strings.  However, this doesn't seem to be necessary for methods like WebCore::HTMLAnchorElement::href().

- WebCore::KURLHash::hash() in KURLHash.h was modified to handle "null" KURL objects (which would otherwise cause a crash if you tried to add one to a hash).

[Darin Adler]

(In reply to comment #3)
> - The change to WebCore::Node::getSubresourceURLs() removes the parseURL()
> method from being called on most (if not all) subresource URL strings. 
> However, this doesn't seem to be necessary for methods like
> WebCore::HTMLAnchorElement::href().

Here's what parseURL does:

    1) Strip leading and trailing spaces.
    2) Strip URL() if it surrounds the string, allowing any mix of uppercase and lowercase.
    3) Strip leading and trailing spaces.
    4) Strip balanced single or double quotes.
    5) Strip leading and trailing spaces.
    6) Strip all characters in the range U+0000-U+000C

It's very strange that we want to do this anywhere except in CSS. In fact, I think it's a bug almost every where this function is used. Perhaps some of this is needed but obviously (2) is something specific to CSS property values even if some of the other behavior is useful.

Someone should create some tests cases and investigate the behavior of other web browsers for the various places we call this function. And at the same time we should probably rename it.

> - WebCore::KURLHash::hash() in KURLHash.h was modified to handle "null" KURL
> objects (which would otherwise cause a crash if you tried to add one to a
> hash).

Changing the hash function in this way won't work. The null KURL is the empty value, so it can't be used as a value in a set. It's not just the hash function. In fact, you should get an assertion in the HashTable implementation if you try to add an empty KURL, and if you don't then there is some sort of bug.

Using a set introduces a sort of randomness (not something truly random, but close enough) into the process that wasn't there before. Perhaps we should sort the URLs in the set so we process them in a predictable order?

[David Kilzer (:ddkilzer)]

Comment on attachment 25753 [details]
Pre-patch #2 v1

(In reply to comment #4)
> (In reply to comment #3)
> > - The change to WebCore::Node::getSubresourceURLs() removes the parseURL()
> > method from being called on most (if not all) subresource URL strings. 
> > However, this doesn't seem to be necessary for methods like
> > WebCore::HTMLAnchorElement::href().
> 
> Here's what parseURL does:
> 
>     1) Strip leading and trailing spaces.
>     2) Strip URL() if it surrounds the string, allowing any mix of uppercase
> and lowercase.
>     3) Strip leading and trailing spaces.
>     4) Strip balanced single or double quotes.
>     5) Strip leading and trailing spaces.
>     6) Strip all characters in the range U+0000-U+000C
> 
> It's very strange that we want to do this anywhere except in CSS. In fact, I
> think it's a bug almost every where this function is used. Perhaps some of this
> is needed but obviously (2) is something specific to CSS property values even
> if some of the other behavior is useful.
> 
> Someone should create some tests cases and investigate the behavior of other
> web browsers for the various places we call this function. And at the same time
> we should probably rename it.

Filed Bug 22664.

> > - WebCore::KURLHash::hash() in KURLHash.h was modified to handle "null" KURL
> > objects (which would otherwise cause a crash if you tried to add one to a
> > hash).
> 
> Changing the hash function in this way won't work. The null KURL is the empty
> value, so it can't be used as a value in a set. It's not just the hash
> function. In fact, you should get an assertion in the HashTable implementation
> if you try to add an empty KURL, and if you don't then there is some sort of
> bug.

I really wanted to avoid adding code to check whether KURL.isNull() every place that adds KURL objects to the list.  I don't see an easy way to do this right now.

> Using a set introduces a sort of randomness (not something truly random, but
> close enough) into the process that wasn't there before. Perhaps we should sort
> the URLs in the set so we process them in a predictable order?

I could use a ListHashSet instead.

Unfortunately I don't have time to fix this clean-up patch, so I filed Bug 22666.

[Darin Adler]

(In reply to comment #5)
> I really wanted to avoid adding code to check whether KURL.isNull() every place
> that adds KURL objects to the list.  I don't see an easy way to do this right
> now.

You could make the set instead be a class and have its add function filter out nulls. You could put a helper function to add elements to the set into the header and use it instead of using the set add function directly and have that filter out nulls.

[David Kilzer (:ddkilzer)]

Created attachment 25821 [details]
Patch v1

Proposed patch.

This will cause any resources referenced in CSS stylesheets (or inline style="" attributes) to be included in webarchives, assuming those resources were used when laying out the page.

[Darin Adler]

Comment on attachment 25821 [details]
Patch v1

Test case seems a little huge, but I'm glad it has thorough coverage.

I'm not sure I like the change to CSSBorderImageValue.h. Normally the concept is that if someone just wants to get at some of the CSSBorderImageValue class definition without actually creating one, they don't need to include one additional header. The file CSSMutableStyleDeclaration.cpp is one example. But I could be wrong -- you might end up having to add tons of includes of "Rect.h"; I'll go along with whatever you decide.

Do we have to add this new test to the skip file for most platforms, since they don't support web archives in this format?

> +void CSSMutableStyleDeclaration::addSubresourceStyleURLs(PassRefPtr<CSSPrimitiveValue> primitiveValue, ListHashSet<KURL>& urls, const KURL& baseURL)

This doesn't take ownership of the value, so it should take a raw pointer rather than a PassRefPtr.

> +    addSubresourceURL(urls, KURL(baseURL, parseURL(value->cssText())));

Is this the right way to construct the URL relative to the base? Don't we have to pass the encoding the way Document::completeURL does? Maybe it would be better to pass in a Document* and use completeURL instead of passing a baseURL.

> +    if (property.value()->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE) {
> +        addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(property.value()), urls, baseURL);
> +        return;
> +    }
> +
> +    if (property.value()->cssValueType() == CSSValue::CSS_VALUE_LIST) {
> +        RefPtr<CSSValueList> valueList = static_cast<CSSValueList*>(property.value());
> +        unsigned max = valueList->length();
> +        for (unsigned i = 0; i < max; ++i) {
> +            if (valueList->itemWithoutBoundsCheck(i)->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE)
> +                addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(valueList->itemWithoutBoundsCheck(i)), urls, baseURL);
> +        }
> +    }

I'd rather see a switch statement here than two if statements.

Also, valueList->itemWithoutBoundsCheck(i) is done twice. I think you should use a local variable instead.

> +                RefPtr<CSSValueList> srcList = static_cast<CSSValueList*>((*it).value());

The syntax it-> should work here -- no need for (*it). It it doesn't work, then I guess it's OK to leave it this way. We'll change it when we eliminate DeprecatedValueList. Actually, I think Antti has already done that so you might have to change if he lands first.

> +                    RefPtr<CSSFontFaceSrcValue> value = static_cast<CSSFontFaceSrcValue*>(srcList->itemWithoutBoundsCheck(i));

This doesn't need to be a RefPtr -- a raw pointer will do fine.

> +                    RefPtr<CSSValue> value = static_cast<CSSBorderImageValue*>((*it).value())->imageValue();

Ditto.

> +                if (value->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE)

Ditto.

> +    RefPtr<CSSMutableStyleDeclaration> style = inlineStyleDecl();

Ditto.

My concerns are all doubts, not definite problems. So review+

[David Kilzer (:ddkilzer)]

Comment on attachment 25821 [details]
Patch v1

Clearing Darin Adler's review+ flag to address issues in Comment #8.  I need to research the encoding issue.  (Does a stand-alone stylesheet have a document()?  It may have its own character encoding, though.)

If I didn't reply to a comment below, I will address it in the next patch.

(In reply to comment #8)
> (From update of attachment 25821 [details] [review])
> Test case seems a little huge, but I'm glad it has thorough coverage.

Yes, I was aiming for complete coverage.  Note that I reused the same apple.gif image in nearly all of the CSS styles but simply added a query string to the end of the URL.

> I'm not sure I like the change to CSSBorderImageValue.h. Normally the concept
> is that if someone just wants to get at some of the CSSBorderImageValue class
> definition without actually creating one, they don't need to include one
> additional header. The file CSSMutableStyleDeclaration.cpp is one example. But
> I could be wrong -- you might end up having to add tons of includes of
> "Rect.h"; I'll go along with whatever you decide.

I looked at this.  Ironically, CSSMutableStyleDeclaration.cpp is the first time that Rect.h would NOT be included with CSSBorderImageValue.h anyway.  I didn't want to #include Rect.h there just because it was needed for CSSBorderImageValue.h.  Here's the compiler error:

/symroots/Release/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h: In destructor ‘WTF::PassRefPtr<T>::~PassRefPtr() [with T = WebCore::Rect]’:
WebCore/css/CSSBorderImageValue.h:36:   instantiated from here
/symroots/Release/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h:43: error: invalid use of incomplete type ‘struct WebCore::Rect’
WebCore/css/CSSPrimitiveValue.h:33: error: forward declaration of ‘struct WebCore::Rect’

> Do we have to add this new test to the skip file for most platforms, since they
> don't support web archives in this format?

No, the SkippedList for all the non-Mac platforms already includes LayoutTests/webarchive.

> > +    addSubresourceURL(urls, KURL(baseURL, parseURL(value->cssText())));
> 
> Is this the right way to construct the URL relative to the base? Don't we have
> to pass the encoding the way Document::completeURL does? Maybe it would be
> better to pass in a Document* and use completeURL instead of passing a baseURL.

Will research this.  For stylesheets declared in <style></style> tags, the document's encoding would be appropriate.  For standalone stylesheets, we'd need to get the encoding of the given stylesheet instead.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (From update of attachment 25821 [details] [review])
> > +                RefPtr<CSSValueList> srcList = static_cast<CSSValueList*>((*it).value());
> 
> The syntax it-> should work here -- no need for (*it). It it doesn't work, then
> I guess it's OK to leave it this way. We'll change it when we eliminate
> DeprecatedValueList. Actually, I think Antti has already done that so you might
> have to change if he lands first.

Unfortunately, it doesn't work:

WebCore/css/CSSMutableStyleDeclaration.cpp:816: error: base operand of ‘->’ has non-pointer type ‘WebCore::DeprecatedValueListConstIterator<WebCore::CSSProperty>’

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 25821 [details] [review])
> > > +    addSubresourceURL(urls, KURL(baseURL, parseURL(value->cssText())));
> > 
> > Is this the right way to construct the URL relative to the base? Don't we have
> > to pass the encoding the way Document::completeURL does? Maybe it would be
> > better to pass in a Document* and use completeURL instead of passing a baseURL.
> 
> Will research this.  For stylesheets declared in <style></style> tags, the
> document's encoding would be appropriate.  For standalone stylesheets, we'd
> need to get the encoding of the given stylesheet instead.

If I understand the code correctly, styles (and stylesheets) are already decoded when I get String values from CSSPrimitiveValue objects, so I don't think it's necessary to pass a TextEncoding object when constructing a KURL from a "url(...)" CSS value.

[Darin Adler]

(In reply to comment #11)
> If I understand the code correctly, styles (and stylesheets) are already
> decoded when I get String values from CSSPrimitiveValue objects, so I don't
> think it's necessary to pass a TextEncoding object when constructing a KURL
> from a "url(...)" CSS value.

This is something we might need to discuss in person.

The reason the encoding is passed in is not for decoding. Things are already decoded even in the web page case. The encoding is used when converting non-ASCII characters back into URLs. For compatibility with old behavior, we convert to the encoding the page originally used, not UTF-8.

This is used to match the behavior of old pre-Unicode web browsers to emulate the behavior you'd see if the URL was expanded in to %-escape sequences *before* decoding. But since we've already decoded it's a little roundabout.

Anyway, this might be a tricky thing to test, and it's probably not critical except for some really old websites. But if we did want to test it we'd just need to construct some non-UTF-8 web pages or pages with non-UTF-8 encodings for style sheets and check the %-escape sequences used in the URLs in other web browsers and in ours.

[David Kilzer (:ddkilzer)]

Created attachment 25854 [details]
Patch v2 (requires more work)

Need to talk to Darin Adler regarding the issue in Comment #12.

Changes since Patch v1:
- Changed CSSMutableStyleDeclaration::addSubresourceStyleURLs(PassRefPtr<CSSPrimitiveValue> primitiveValue, ListHashSet<KURL>& urls, const KURL& baseURL) to make first argument a simple pointer.
- Removed use of unnecessary RefPtr<> in CSSMutableStyleDeclaration::addSubresourceStyleURLs(const CSSProperty& property, ListHashSet<KURL>& urls, const KURL& baseURL).  Also changed to use switch() statement.
- Updated CSSMutableStyleDeclaration::addSubresourceStyleURLs(ListHashSet<KURL>& urls, const KURL& baseURL) to remove use of DeprecatedValueListConstIterator after Antti's recent CSS changes.
- Removed use of unnecessary RefPtr<> in StyledElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls).

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > If I understand the code correctly, styles (and stylesheets) are already
> > decoded when I get String values from CSSPrimitiveValue objects, so I don't
> > think it's necessary to pass a TextEncoding object when constructing a KURL
> > from a "url(...)" CSS value.
> 
> This is something we might need to discuss in person.
> 
> The reason the encoding is passed in is not for decoding. Things are already
> decoded even in the web page case. The encoding is used when converting
> non-ASCII characters back into URLs. For compatibility with old behavior, we
> convert to the encoding the page originally used, not UTF-8.
> 
> This is used to match the behavior of old pre-Unicode web browsers to emulate
> the behavior you'd see if the URL was expanded in to %-escape sequences
> *before* decoding. But since we've already decoded it's a little roundabout.
> 
> Anyway, this might be a tricky thing to test, and it's probably not critical
> except for some really old websites. But if we did want to test it we'd just
> need to construct some non-UTF-8 web pages or pages with non-UTF-8 encodings
> for style sheets and check the %-escape sequences used in the URLs in other web
> browsers and in ours.

The KURL objects being constructed are for the sole purpose of retrieving an ArchiveResource from DocumentLoader::subresource() or a CachedResource from the WebCore::Cache in LegacyWebArchive::create(const String&, Frame*, Vector<Node*>&).

Do both of these mechanisms obey this encoding for url() resources in inline styles and stylesheets?

Are these web sites so old as to pre-date the invention of the CSS stylesheet?

I guess I'll talk to you in person about this.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > If I understand the code correctly, styles (and stylesheets) are already
> > decoded when I get String values from CSSPrimitiveValue objects, so I don't
> > think it's necessary to pass a TextEncoding object when constructing a KURL
> > from a "url(...)" CSS value.
> 
> This is something we might need to discuss in person.
> 
> The reason the encoding is passed in is not for decoding. Things are already
> decoded even in the web page case. The encoding is used when converting
> non-ASCII characters back into URLs. For compatibility with old behavior, we
> convert to the encoding the page originally used, not UTF-8.
> 
> This is used to match the behavior of old pre-Unicode web browsers to emulate
> the behavior you'd see if the URL was expanded in to %-escape sequences
> *before* decoding. But since we've already decoded it's a little roundabout.
> 
> Anyway, this might be a tricky thing to test, and it's probably not critical
> except for some really old websites. But if we did want to test it we'd just
> need to construct some non-UTF-8 web pages or pages with non-UTF-8 encodings
> for style sheets and check the %-escape sequences used in the URLs in other web
> browsers and in ours.

Safari (3.2.1 and ToT WebKit), Firefox (3) and MSIE (7) all have different behavior when encoding a URL from a CSS stylesheet that is sent with an "interesting" encoding like shift-jis.

The test cases are as follows.  All HTML documents use <meta> tags to specify different encodings.  The CSS document uses an Apache .htaccess file to specify "text/css;charset=shift-jis" encoding.  Each HTML document includes an <img> tag with a shift-jis character (<88><9F>) and includes the CSS document using a <link> tag.  The CSS document includes the same shift-jis character (<88><9F>).  Files were served from a Mac OS X 10.5.5/10.5.6 system running Apache 2.

test-css-url-encoding.html: No charset; plain text file (no BOM).
test-css-url-encoding-shift-jis.html: Uses shift-jis charset; plain text file (no BOM).
test-css-url-encoding-utf-8.html:  Uses UTF-8 charset; UTF-8 file with BOM.
resources/test-shift-jis.css: Plain text file (no BOM).


Firefox 3.0.5 on Mac OS X 10.5.5:

test-css-url-encoding.html: %CB%86%C5%B8
resources/test-shift-jis.css: %E4%BA%9C

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-shift-jis.css: %88%9F

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-shift-jis.css: %E4%BA%9C


MSIE 7 on Windows XP SP2:

test-css-url-encoding.html: \x88\x9f
resources/test-shift-jis.css: \x88\x9f

test-css-url-encoding-shift-jis.html: \x88\x9f
resources/test-shift-jis.css: \x88\x9f

test-css-url-encoding-utf-8.html: \xe4\xba\x9c
resources/test-shift-jis.css: \xef\xbf\xbd\xef\xbf\xbd


Safari 3.2.1 on Mac OS X 10.5.6:

test-css-url-encoding.html: %88%9F
resources/test-shift-jis.css: %E4%BA%9C

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-shift-jis.css: %E4%BA%9C

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-shift-jis.css: %E4%BA%9C


With the next patch I post to this bug, ToT WebKit behavior changes to this:

test-css-url-encoding.html: %88%9F
resources/test-shift-jis.css: %88%9F

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-shift-jis.css: %88%9F

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-shift-jis.css: %88%9F

[David Kilzer (:ddkilzer)]

Created attachment 26172 [details]
Test files for Comment #15

These are the test files used for the tests described in Comment #15.

They are rooted in LayoutTests/http/tests/webarchive/ within the WebKit source directory.

[David Kilzer (:ddkilzer)]

Created attachment 26177 [details]
Test files for Comment #17

Okay, let's try this again with more *.css variants:

test-css-url-encoding.html: No charset; plain text file (no BOM).
test-css-url-encoding-shift-jis.html: Uses shift-jis charset; plain text file (no BOM).
test-css-url-encoding-utf-8.html:  Uses UTF-8 charset; UTF-8 file with BOM.
resources/test-no-charset.css: No charset; plain text file (no BOM).
resources/test-shift-jis.css: Uses shift-jis charset; lain text file (no BOM).
resources/test-utf-8.css: Uses UTF-8 charset; UTF-8 file with BOM.


Firefox 3.0.5 on Mac OS X 10.5.6:

test-css-url-encoding.html: %CB%86%C5%B8
resources/test-no-charset.css: %CB%86%C5%B8
resources/test-shift-jis.css: %E4%BA%9C
resources/test-utf-8.css: %E4%BA%9C

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-no-charset.css: %88%9F
resources/test-shift-jis.css: %88%9F
resources/test-utf-8.css: %88%9F

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD
resources/test-shift-jis.css: %E4%BA%9C
resources/test-utf-8.css: %E4%BA%9C


MSIE 7 on Windows XP SP2:

test-css-url-encoding.html: \x88\x9f
resources/test-no-charset.css: \x88\x9f
resources/test-shift-jis.css: \x88\x9f
resources/test-utf-8.css: ?        <-- as printed to console

test-css-url-encoding-shift-jis.html: \x88\x9f
resources/test-no-charset.css: \x88\x9f
resources/test-shift-jis.css: \x88\x9f
resources/test-utf-8.css: \x88\x9f

test-css-url-encoding-utf-8.html: \xe4\xba\x9c
resources/test-no-charset.css: \xef\xbf\xbd\xef\xbf\xbd
resources/test-shift-jis.css: \xef\xbf\xbd\xef\xbf\xbd
resources/test-utf-8.css: \xe4\xba\x9c


Safari 3.2.1 on Mac OS X 10.5.6:

test-css-url-encoding.html: %88%9F
resources/test-no-charset.css: %CB%86%C5%B8
resources/test-shift-jis.css: %E4%BA%9C
resources/test-utf-8.css: %E4%BA%9C

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-no-charset.css: %E4%BA%9C
resources/test-shift-jis.css: %E4%BA%9C
resources/test-utf-8.css: %E4%BA%9C

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD
resources/test-shift-jis.css: %E4%BA%9C
resources/test-utf-8.css: %E4%BA%9C


With the next patch I post to this bug, ToT WebKit behavior changes to this:

test-css-url-encoding.html: %88%9F
resources/test-no-charset.css: %88%9F
resources/test-shift-jis.css: %88%9F
resources/test-utf-8.css: %E4%BA%9C

test-css-url-encoding-shift-jis.html: %88%9F
resources/test-no-charset.css: %88%9F
resources/test-shift-jis.css: %88%9F
resources/test-utf-8.css: %E4%BA%9C

test-css-url-encoding-utf-8.html: %E4%BA%9C
resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD
resources/test-shift-jis.css: %88%9F
resources/test-utf-8.css: %E4%BA%9C

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> With the next patch I post to this bug, ToT WebKit behavior changes to this:
> 
> test-css-url-encoding.html: %88%9F
> resources/test-no-charset.css: %88%9F
> resources/test-shift-jis.css: %88%9F
> resources/test-utf-8.css: %E4%BA%9C
> 
> test-css-url-encoding-shift-jis.html: %88%9F
> resources/test-no-charset.css: %88%9F
> resources/test-shift-jis.css: %88%9F
> resources/test-utf-8.css: %E4%BA%9C
> 
> test-css-url-encoding-utf-8.html: %E4%BA%9C
> resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD

This encoding is exposing a bug which I need to investigate.  This encoding does not appear in the webarchive output.

> resources/test-shift-jis.css: %88%9F
> resources/test-utf-8.css: %E4%BA%9C

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (In reply to comment #17)
> > With the next patch I post to this bug, ToT WebKit behavior changes to this:
> > 
> > test-css-url-encoding.html: %88%9F
> > resources/test-no-charset.css: %88%9F
> > resources/test-shift-jis.css: %88%9F
> > resources/test-utf-8.css: %E4%BA%9C
> > 
> > test-css-url-encoding-shift-jis.html: %88%9F
> > resources/test-no-charset.css: %88%9F
> > resources/test-shift-jis.css: %88%9F
> > resources/test-utf-8.css: %E4%BA%9C
> > 
> > test-css-url-encoding-utf-8.html: %E4%BA%9C
> > resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD
> 
> This encoding is exposing a bug which I need to investigate.  This encoding
> does not appear in the webarchive output.

HTMLLinkElement::process() was setting the encoding of the stylesheet to the document's encoding if the stylesheet did not specify an encoding in HTML.  Is this correct?

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > With the next patch I post to this bug, ToT WebKit behavior changes to this:
> > > 
> > > test-css-url-encoding.html: %88%9F
> > > resources/test-no-charset.css: %88%9F
> > > resources/test-shift-jis.css: %88%9F
> > > resources/test-utf-8.css: %E4%BA%9C
> > > 
> > > test-css-url-encoding-shift-jis.html: %88%9F
> > > resources/test-no-charset.css: %88%9F
> > > resources/test-shift-jis.css: %88%9F
> > > resources/test-utf-8.css: %E4%BA%9C
> > > 
> > > test-css-url-encoding-utf-8.html: %E4%BA%9C
> > > resources/test-no-charset.css: %EF%BF%BD%EF%BF%BD
> > 
> > This encoding is exposing a bug which I need to investigate.  This encoding
> > does not appear in the webarchive output.
> 
> HTMLLinkElement::process() was setting the encoding of the stylesheet to the
> document's encoding if the stylesheet did not specify an encoding in HTML.  Is
> this correct?

This code was added for Bug 11011 with test fast/encoding/css-charset-default.xhtml.

http://trac.webkit.org/changeset/16689

The bug seems to be in the CSSStyleSheet::completeURL() method as it's not getting the correct charset when encoding the URL.

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> (In reply to comment #19)
> > HTMLLinkElement::process() was setting the encoding of the stylesheet to the
> > document's encoding if the stylesheet did not specify an encoding in HTML.  Is
> > this correct?
> 
> This code was added for Bug 11011 with test
> fast/encoding/css-charset-default.xhtml.
> 
> http://trac.webkit.org/changeset/16689
> 
> The bug seems to be in the CSSStyleSheet::completeURL() method as it's not
> getting the correct charset when encoding the URL.

Actually, it's a bug in DumpRenderTree.  When http/tests/webarchive/test-css-url-encoding-utf-8.html is run by itself, it produces the correct results.  When it's run after http/tests/webarchive/test-css-url-encoding.html and http/tests/webarchive/test-css-url-encoding-shift-jis.html, it produces incorrect results.

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > HTMLLinkElement::process() was setting the encoding of the stylesheet to the
> > > document's encoding if the stylesheet did not specify an encoding in HTML.  Is
> > > this correct?
> > 
> > This code was added for Bug 11011 with test
> > fast/encoding/css-charset-default.xhtml.
> > 
> > http://trac.webkit.org/changeset/16689
> > 
> > The bug seems to be in the CSSStyleSheet::completeURL() method as it's not
> > getting the correct charset when encoding the URL.
> 
> Actually, it's a bug in DumpRenderTree.  When
> http/tests/webarchive/test-css-url-encoding-utf-8.html is run by itself, it
> produces the correct results.  When it's run after
> http/tests/webarchive/test-css-url-encoding.html and
> http/tests/webarchive/test-css-url-encoding-shift-jis.html, it produces
> incorrect results.

The actual issue is in how the charset for the test-no-charset.css stylesheet is cached in WebCore.  The first time it's loaded, it picks up the charset of the including HTML document.  Thereafter its charset apparently never changes because it's only set the first time it's loaded.  (This is visible rarely in Safari 3.2.1 on Mac OS X 10.5.6, but happens consistently across page reloads in ToT WebKit.)

This means that if two different web pages with two different charsets load the same CSS stylesheet with no charset, the first one always "wins" because it sets the charset for the CSS stylesheet.  The second page will incorrectly decode the URLs (and non-ASCII characters) in the CSS stylesheet until the WebCore cache is cleared.  This breaks item #4 in CSS 2.1:

http://www.w3.org/TR/CSS21/syndata.html#charset

There are a couple of workarounds for the failing test, but they don't address the actual bug:

1. Add a query parameter to the end of the test-no-charset.css URL so cached copies won't be used.
2. Clear the WebCore cache in DumpRenderTree after every test.

I filed Bug 22952 for this issue.

[David Kilzer (:ddkilzer)]

Created attachment 26184 [details]
Patch v3

NOTE: The bulk of this patch is layout test source and output.

Changes since Patch v2:

- Added StyleSheet::completeURL() and CSSStyleSheet::completeURL() with tests to address concerns in Comment #12.  Switched CSSParser.cpp to use completeURL().  See also Comment #17.

- Removed baseURL parameter from being passed around now that CSSStyleSheet::completeURL() exists.

- Fixed indentation of switch() statement in CSSMutableStyleDeclaration::addSubresourceStyleURLs().

- Made StyleBase::stylesheet() const.

[Darin Adler]

Comment on attachment 26184 [details]
Patch v3

This looks really good!

I have two major suggestions:

    1) Maybe this code could be written to use more virtual functions. In CSSValue and CSSRule. To avoid the switch statements on types of value and types of rule.

    2) Perhaps this could be written to ignore the property ID and do everything based on the type of value rather than the property. I think that would be more robust against changes to CSS; is there a reason not to do it that way?

Many of my comments below about switch statement style and type vs. is functions will be irrelevant if you use virtual functions instead of switch statements.

> +        RefPtr<CSSValueList> valueList = static_cast<CSSValueList*>(property.value());

No need to use a RefPtr here; the local code doesn't have to take ownership of the list; this should just be a raw pointer. I see that mistake in multiple places in this new code, and it's unnecessarily inefficient. (More detailed discussion below.)

> +        unsigned max = valueList->length();

To my taste, it's a little strange to call this "max". My general rule is to adopt the same naming as the class in question. So if CSSValueList calls this length(), then I call the local variable length. I won't repeat this comment about other variables named "max" in the patch, but it applies to them as well.

> +            if (value->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE)
> +                addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(value), urls, styleSheet);

I think it might be better to use isPrimitiveValue() instead of cssValueType(), since there's no switch statement involved and the syntax would be easier to read. But also I don't understand why primitive values are the only types that could have URLs in them. What about font values, for example, and image values? As I mentioned above, I'd prefer an approach that lets the rules and values handle things with virtual functions and have fewer assumptions about what types can appear in lists, and what types might have URLs.

> +    default:
> +        break;

My usual approach in switch statements like this is to list the types that I'm not handling, to take advantage of gcc's warning about missing cases. That way if a new enum value is added later, the person adding it will be reminded to consider whether to include a case at this site, because they'll have to fix a compiler warning by adding a case to the switch for the new type. Including "default: break" prevents that warning. Would you consider listing the other enum types instead? One good thing about having those cases is that you can then have a comment explaining why those types can be skipped.

> +    RefPtr<CSSStyleSheet> styleSheet = static_cast<CSSStyleSheet*>(stylesheet());

Should be a raw pointer, not RefPtr (see explanation above). Also, the only reason there's no name conflict here is case (styleSheet vs. stylesheet). It's annoying we have a mix of cases for this term!

> +            RefPtr<CSSValueList> srcList = static_cast<CSSValueList*>(property.value());

Another case where it should be a raw pointer.

> +                RefPtr<CSSFontFaceSrcValue> value = static_cast<CSSFontFaceSrcValue*>(srcList->itemWithoutBoundsCheck(i));

And another.

> +        default:
> +            break;

In this case, the property ID switch statement, I think we *do* need a default, because there are simply too many other properties to list. But I'd like a comment here saying something specific about wby the other properties aren't listed here and explaining which types of properties would need to be added to the switch in the future. But this is an argument in favor of a design that ignores the property IDs and does the whole thing based on the CSS values instead, suggestion (2) above.

> +    static void addSubresourceStyleURLs(CSSPrimitiveValue*, ListHashSet<KURL>&, const CSSStyleSheet* styleSheet);
> +    static void addSubresourceStyleURLs(const CSSProperty& property, ListHashSet<KURL>&, const CSSStyleSheet* styleSheet);

You should omit the argument name for the CSSProperty and CSSStyleSheet arguments in these declarations. The type speaks for itself.

> +    typedef ListHashSet<RefPtr<CSSStyleSheet> > CSSStyleSheetList;

I think this list can collect raw pointers instead of RefPtr. The only reason you'd need to use RefPtr would be if the code does something that can mutate the DOM or CSS during the loop. Accumulating URLs in a collection should not fall into that category. And raw pointers are more efficient.

> +        RefPtr<CSSStyleSheet> styleSheet = *it;

Again, raw pointer please.

> +            RefPtr<CSSRule> rule = ruleList->item(i);

Raw pointer.

> +            switch (rule->type()) {

Consider having virtual functions in CSSRule instead of using a switch statement here, suggestion (2) above.

> +            default:
> +                break;

Same comment about default as above.

> -StyleSheet* StyleBase::stylesheet()
> +StyleSheet* StyleBase::stylesheet() const

In general, I think it's a bad idea to have const member functions in classes that are part of a tree such as the DOM and render trees. We have a lot of them, and I'd like to cut down on that. If you have a const pointer to one element in the tree, and look at its parent, then you end up with non-const pointer so const doesn't really work all that well. I'd rather move in the direction of less const rather than more in the future. So if it was me, I would not add const here.

> +void StyledElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
> +{
> +    CSSMutableStyleDeclaration* style = inlineStyleDecl();
> +    if (style)
> +        style->addSubresourceStyleURLs(urls);
> +}

Putting the definition inside the if statement can be good for a function like this:

    if (CSSMutableStyleDeclaration* style = inlineStyleDecl())
        style->addSubresourceStyleURLs(urls);

Please consider that style for cases like this when it's practical. One benefit is that you can't accidentally use the null pointer because when it's null it's not even in scpe.

>  void HTMLEmbedElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
>  {
> +    StyledElement::addSubresourceAttributeURLs(urls);
> +
>      addSubresourceURL(urls, document()->completeURL(src()));
>  }

The normal style here is to name the base class when calling the base class's version of the function, so in this case the call would be HTMLPlugInImageElement::addSubresourceAttributeURLs. It's true that the base class doesn't override this function and so the one that will actually be called is the one in StyledElement, but calling for StyledElement explicitly has the unusual effect of skipping overrides in intervening classes. Although there are no such overrides now, they could be added later. Calling directly to a class farther up the hierarchy should normally only be done when there's a specific reason skip some clases.

Same comment about the other addSubresourceAttributeURLs functions such as the one in HTMLBodyElement, etc.

>      StyleSheet* styleSheet = const_cast<HTMLStyleElement*>(this)->sheet();
>      if (styleSheet)
> -        styleSheet->addSubresourceStyleURLs(urls, ownerDocument()->baseURL());
> +        styleSheet->addSubresourceStyleURLs(urls);

Same comment about putting the definition inside the if statement as above.

And here's yet another example where const is not serving us well! All those const_cast start to get annoying and I don't think the const is providing much benefit.

This patch is good, and could be landed as-is, r=me.

But ideally you'd resolve some of the things I mention in this comment either before or after landing the changes. I'll leave it up to you know to handle that.

[David Kilzer (:ddkilzer)]

Comment on attachment 26184 [details]
Patch v3

Clearing Darin Adler's review+ flag to update patch.

Will address all the feedback in Comment #24, including the addition of virtual methods to clean up the switch statements.

[David Kilzer (:ddkilzer)]

Created attachment 26215 [details]
Patch v4

Changes since Patch v3:

- Reverted const-ness of StyleBase::stylesheet().
- Removed const-ness from all addSubresourceStyleURLs() methods.
- Replaced switch() statements with virtual methods on CSSRule and CSSValue subclasses (OO design FTW!).
- Removed abuse of RefPtr<> in favor of raw pointers.
- Renamed max variables.
- Inlined null-checked variable declarations into if() statements.
- Called immediate superclass instead of StyledElement::addSubresourceAttributeURLs().
- Reverted changes to CSSBorderImageValue.h to include Rect.h (although it only works because each source file where it's included also includes Rect.h).
- Tweaked ProcessingInstruction::addSubresourceAttributeURLs() to call StyleSheet::baseURL().

[Darin Adler]

Comment on attachment 26215 [details]
Patch v4

> +    if (!isLocal())
> +        addSubresourceURL(urls, styleSheet->completeURL(m_resource));

Is the isLocal() check an optimization or a correctness thing?

> --- a/WebCore/css/CSSPrimitiveValue.cpp
> +++ b/WebCore/css/CSSPrimitiveValue.cpp
> @@ -23,10 +23,12 @@
>  
>  #include "CSSHelper.h"
>  #include "CSSPropertyNames.h"
> +#include "CSSStyleSheet.h"
>  #include "CSSValueKeywords.h"
>  #include "Color.h"
>  #include "Counter.h"
>  #include "ExceptionCode.h"
> +#include "Node.h"
>  #include "Pair.h"
>  #include "Rect.h"
>  #include "RenderStyle.h"

Why the include of Node.h?

> +    while (styleSheetList.size() > 0) {
> +        CSSStyleSheetList::iterator it = styleSheetList.begin();
> +        CSSStyleSheet* styleSheet = *it;
> +        styleSheetList.remove(it);

It seems like a mistake to remove style sheets from the list, because then you could end up processing the same sheet twice. Or maybe this should just be a Deque rather than a ListHashSet.

> +            if (rule->isImportRule())
> +                if (CSSStyleSheet* ruleStyleSheet = static_cast<CSSImportRule*>(rule)->styleSheet())
> +                    styleSheetList.add(ruleStyleSheet);

Need braces around this multiple line if statement.

> +    unsigned size = m_values.size();
> +    for (unsigned i = 0; i < size; ++i)
> +        m_values[i]->addSubresourceStyleURLs(urls, styleSheet);

Should be size_t rather than unsigned.

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #27)
> (From update of attachment 26215 [details] [review])
> > +    if (!isLocal())
> > +        addSubresourceURL(urls, styleSheet->completeURL(m_resource));
> 
> Is the isLocal() check an optimization or a correctness thing?

Correctness.  A local font source is the name of the font, not a URL.  See CSSFontFaceSrcValue::cssText().

> > --- a/WebCore/css/CSSPrimitiveValue.cpp
> > +++ b/WebCore/css/CSSPrimitiveValue.cpp
> > @@ -23,10 +23,12 @@
> >  
> >  #include "CSSHelper.h"
> >  #include "CSSPropertyNames.h"
> > +#include "CSSStyleSheet.h"
> >  #include "CSSValueKeywords.h"
> >  #include "Color.h"
> >  #include "Counter.h"
> >  #include "ExceptionCode.h"
> > +#include "Node.h"
> >  #include "Pair.h"
> >  #include "Rect.h"
> >  #include "RenderStyle.h"
> 
> Why the include of Node.h?

It's included for the inlined addSubresourceURL() method which needed a home.  It's included everywhere else it's needed (directly or indirectly).

> > +    while (styleSheetList.size() > 0) {
> > +        CSSStyleSheetList::iterator it = styleSheetList.begin();
> > +        CSSStyleSheet* styleSheet = *it;
> > +        styleSheetList.remove(it);
> 
> It seems like a mistake to remove style sheets from the list, because then you
> could end up processing the same sheet twice. Or maybe this should just be a
> Deque rather than a ListHashSet.

As it turns out, if stylesheet A includes stylesheet B, then B includes A again, the CSSImportRule for importing A within B doesn't have a stylesheet attached to it, so this doesn't recurse needlessly.  (The original implementation of CSSStyleSheet::addSubresourceStyleURLs() called itself recursively, which worked because of this detail.)

Should I switch it to a Deque anyway (in a follow-up patch)?

> > +            if (rule->isImportRule())
> > +                if (CSSStyleSheet* ruleStyleSheet = static_cast<CSSImportRule*>(rule)->styleSheet())
> > +                    styleSheetList.add(ruleStyleSheet);
> 
> Need braces around this multiple line if statement.

Okay.

> > +    unsigned size = m_values.size();
> > +    for (unsigned i = 0; i < size; ++i)
> > +        m_values[i]->addSubresourceStyleURLs(urls, styleSheet);
> 
> Should be size_t rather than unsigned.

Okay.

[David Kilzer (:ddkilzer)]

(In reply to comment #27)
> (From update of attachment 26215 [details] [review])
> > +    unsigned size = m_values.size();
> > +    for (unsigned i = 0; i < size; ++i)
> > +        m_values[i]->addSubresourceStyleURLs(urls, styleSheet);
> 
> Should be size_t rather than unsigned.

Note that CSSValueList::createParserValueList() uses unsigned as well, as does CSSValueList::cssText(), CSSValueList::item(unsigned) and CSSValueList::CSSValueList(CSSParserValueList*).

[Darin Adler]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 26215 [details] [review] [review])
> > > +    unsigned size = m_values.size();
> > > +    for (unsigned i = 0; i < size; ++i)
> > > +        m_values[i]->addSubresourceStyleURLs(urls, styleSheet);
> > 
> > Should be size_t rather than unsigned.
> 
> Note that CSSValueList::createParserValueList() uses unsigned as well, as does
> CSSValueList::cssText(), CSSValueList::item(unsigned) and
> CSSValueList::CSSValueList(CSSParserValueList*).

My point is only that if you're iterating a Vector you should use size_t. Not saying you should use it for all indexing everywhere.

[Darin Adler]

(In reply to comment #28)
> > > +    while (styleSheetList.size() > 0) {
> > > +        CSSStyleSheetList::iterator it = styleSheetList.begin();
> > > +        CSSStyleSheet* styleSheet = *it;
> > > +        styleSheetList.remove(it);
> > 
> > It seems like a mistake to remove style sheets from the list, because then you
> > could end up processing the same sheet twice. Or maybe this should just be a
> > Deque rather than a ListHashSet.
> 
> As it turns out, if stylesheet A includes stylesheet B, then B includes A
> again, the CSSImportRule for importing A within B doesn't have a stylesheet
> attached to it, so this doesn't recurse needlessly.  (The original
> implementation of CSSStyleSheet::addSubresourceStyleURLs() called itself
> recursively, which worked because of this detail.)
> 
> Should I switch it to a Deque anyway (in a follow-up patch)?

Yes, I think so. We shouldn't use a ListHashSet if we don't really need the uniqueness property it provides.

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/webarchive/resources/.htaccess
	A	LayoutTests/http/tests/webarchive/resources/apple.gif
	A	LayoutTests/http/tests/webarchive/resources/test-no-charset.css
	A	LayoutTests/http/tests/webarchive/resources/test-shift-jis.css
	A	LayoutTests/http/tests/webarchive/resources/test-utf-8.css
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding-expected.webarchive
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding-shift-jis-expected.webarchive
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding-shift-jis.html
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding-utf-8-expected.webarchive
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding-utf-8.html
	A	LayoutTests/http/tests/webarchive/test-css-url-encoding.html
	A	LayoutTests/webarchive/resources/Ahem.ttf
	A	LayoutTests/webarchive/test-css-url-resources-in-stylesheets-expected.webarchive
	A	LayoutTests/webarchive/test-css-url-resources-in-stylesheets.html
	A	LayoutTests/webarchive/test-css-url-resources-inline-styles-expected.webarchive
	A	LayoutTests/webarchive/test-css-url-resources-inline-styles.html
	M	WebCore/ChangeLog
	M	WebCore/css/CSSBorderImageValue.cpp
	M	WebCore/css/CSSBorderImageValue.h
	M	WebCore/css/CSSFontFaceRule.cpp
	M	WebCore/css/CSSFontFaceRule.h
	M	WebCore/css/CSSFontFaceSrcValue.cpp
	M	WebCore/css/CSSFontFaceSrcValue.h
	M	WebCore/css/CSSImportRule.cpp
	M	WebCore/css/CSSImportRule.h
	M	WebCore/css/CSSMutableStyleDeclaration.cpp
	M	WebCore/css/CSSMutableStyleDeclaration.h
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSPrimitiveValue.cpp
	M	WebCore/css/CSSPrimitiveValue.h
	M	WebCore/css/CSSReflectValue.cpp
	M	WebCore/css/CSSReflectValue.h
	M	WebCore/css/CSSRule.h
	M	WebCore/css/CSSStyleRule.cpp
	M	WebCore/css/CSSStyleRule.h
	M	WebCore/css/CSSStyleSheet.cpp
	M	WebCore/css/CSSStyleSheet.h
	M	WebCore/css/CSSValue.h
	M	WebCore/css/CSSValueList.cpp
	M	WebCore/css/CSSValueList.h
	M	WebCore/css/StyleSheet.cpp
	M	WebCore/css/StyleSheet.h
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/ProcessingInstruction.cpp
	M	WebCore/dom/StyledElement.cpp
	M	WebCore/dom/StyledElement.h
	M	WebCore/html/HTMLBodyElement.cpp
	M	WebCore/html/HTMLEmbedElement.cpp
	M	WebCore/html/HTMLImageElement.cpp
	M	WebCore/html/HTMLInputElement.cpp
	M	WebCore/html/HTMLLinkElement.cpp
	M	WebCore/html/HTMLObjectElement.cpp
	M	WebCore/html/HTMLParamElement.cpp
	M	WebCore/html/HTMLScriptElement.cpp
	M	WebCore/html/HTMLStyleElement.cpp
	M	WebCore/html/HTMLTableCellElement.cpp
	M	WebCore/html/HTMLTableElement.cpp
	M	WebCore/svg/SVGCursorElement.cpp
	M	WebCore/svg/SVGFEImageElement.cpp
	M	WebCore/svg/SVGImageElement.cpp
	M	WebCore/svg/SVGScriptElement.cpp
Committed r39441

http://trac.webkit.org/changeset/39441

Keeping open to fix Comment #31.

[David Kilzer (:ddkilzer)]

Comment on attachment 26215 [details]
Patch v4

Clearing Darin Adler's review flag since this has landed.

[David Kilzer (:ddkilzer)]

Created attachment 26219 [details]
Use Deque in CSSStyleSheet::addSubresourceStyleURLs()

[Darin Adler]

Comment on attachment 26219 [details]
Use Deque in CSSStyleSheet::addSubresourceStyleURLs()

r=me

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSheet.cpp
Committed r39443

http://trac.webkit.org/changeset/39443

[David Kilzer (:ddkilzer)]

Created attachment 26222 [details]
Apache log various browsers loading test pages

This is file that I used to save the results of loading test files in different browsers:

test-css-url-encoding.html
test-css-url-encoding-shift-jis.html
test-css-url-encoding-utf-8.html

See Comment #17 for the results of the test.