Bug 143784 – Add a console message when a stylesheet is not parsed due to invalid MIME type

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

After <http://trac.webkit.org/changeset/180020>, we no longer have a quirks mode exception for CSS MIME types. This means that we'll start rejecting stylesheets that were previously accepted due to this quirk. I think we should log a console message in this case to help Web developers understand why their stylesheet is being rejected.

[Chris Dumez]

Created attachment 250847 [details]
Patch

[Chris Dumez]

Comment on attachment 250847 [details]
Patch

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

> LayoutTests/http/tests/inspector/css/bad-mime-type-expected.txt:6
> +INFO: Unknown issue source:,security

Joe, any idea why this is happening? This looks like a pre-existing issue. It looks like IssueMessage.js does not handle "security" as source. Also, the leading comma looks really weird.

[Chris Dumez]

Created attachment 250852 [details]
Patch

[Build Bot]

Comment on attachment 250852 [details]
Patch

Attachment 250852 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4950895901016064

New failing tests:
http/tests/security/cross-origin-css.html

[Build Bot]

Created attachment 250861 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250852 [details]
Patch

Attachment 250852 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6565140031864832

New failing tests:
http/tests/security/cross-origin-css.html

[Build Bot]

Created attachment 250862 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

> Joe, any idea why this is happening? This looks like a pre-existing issue.
> It looks like IssueMessage.js does not handle "security" as source. Also,
> the leading comma looks really weird.

Yep, I know why this is happening and I'll address it right now twofold:
<https://webkit.org/b/143801> Web Inspector: InspectorTest frontend console methods redirected to the frontend are wrong
<https://webkit.org/b/143803> Web Inspector: Handle all possible Console message Source types in IssueMessage

[Chris Dumez]

Created attachment 250873 [details]
Patch

[Chris Dumez]

Created attachment 250884 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 250884 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:11
> +        previously accepted due to this quirk. I think we should log a console
> +        message in this case to help Web developers understand why their

In the ChangeLog you can drop the "I think we should" phrasing. That is what this change is doing! You can just state it!

    In case this happens, we log a console message to help web developers...

> Source/WebCore/css/StyleSheetContents.cpp:288
> -void StyleSheetContents::parseAuthorStyleSheet(const CachedCSSStyleSheet* cachedStyleSheet, const SecurityOrigin* securityOrigin)
> +void StyleSheetContents::parseAuthorStyleSheet(const CachedCSSStyleSheet* cachedStyleSheet, const SecurityOrigin*)

Since we no longer use this securityOrigin parameter we should get rid of it in the signature and at all the call sites. And I believe that means you can remove the #include as well.

> LayoutTests/http/tests/security/cross-origin-css-4.html:13
> +/* Check that data: is still allowed for non-CORS cross-origin image fetches. */

Perhaps these comments could just be <p> included in the output? Would make it much more obvious if the test fails what it was trying to test without having to find the corresponding html file.

> LayoutTests/http/tests/security/cross-origin-css-4.html:24
> +var onloadTest = async_test("Testing cross-origin and MIME behavior for CSS.");

For instance, replacing or extending this line with those details.

[Chris Dumez]

Created attachment 250888 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250888 [details]
Patch

Clearing flags on attachment: 250888

Committed r182877: <http://trac.webkit.org/changeset/182877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

These new tests are flaky, e.g.: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r182877%20(3549)/http/tests/security/cross-origin-css-1-diff.txt

-CONSOLE MESSAGE: Did not parse stylesheet at 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/security/resources/xorigincss1.html' because its MIME type was invalid.
+CONSOLE MESSAGE: line 9: Did not parse stylesheet at 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/security/resources/xorigincss1.html' because its MIME type was invalid.

I thought that the inconsistent line number detection was fixed for good, but apparently not :(

[Chris Dumez]

(In reply to comment #15)
> These new tests are flaky, e.g.:
> https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> r182877%20(3549)/http/tests/security/cross-origin-css-1-diff.txt
> 
> -CONSOLE MESSAGE: Did not parse stylesheet at
> 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> security/resources/xorigincss1.html' because its MIME type was invalid.
> +CONSOLE MESSAGE: line 9: Did not parse stylesheet at
> 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> security/resources/xorigincss1.html' because its MIME type was invalid.
> 
> I thought that the inconsistent line number detection was fixed for good,
> but apparently not :(

:'( I'll see if I can reproduce.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > These new tests are flaky, e.g.:
> > https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> > r182877%20(3549)/http/tests/security/cross-origin-css-1-diff.txt
> > 
> > -CONSOLE MESSAGE: Did not parse stylesheet at
> > 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> > security/resources/xorigincss1.html' because its MIME type was invalid.
> > +CONSOLE MESSAGE: line 9: Did not parse stylesheet at
> > 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> > security/resources/xorigincss1.html' because its MIME type was invalid.
> > 
> > I thought that the inconsistent line number detection was fixed for good,
> > but apparently not :(
> 
> :'( I'll see if I can reproduce.

I cannot reproduce. However, it looks like the relevant code is in getParserLocationForConsoleMessage() from PageConsoleClient.cpp. It looks like a line number is computed if we're still parsing the HTML document when we receive the CSS resource and try to parse it.

For the CSS to get loaded while we're still parsing the link element, it has to mean that the CSS resource was in the memory cache when requested. As a result, my proposal would be to clear the memory cache using internals.clearMemoryCache() at the beginning of these tests.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > These new tests are flaky, e.g.:
> > > https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> > > r182877%20(3549)/http/tests/security/cross-origin-css-1-diff.txt
> > > 
> > > -CONSOLE MESSAGE: Did not parse stylesheet at
> > > 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> > > security/resources/xorigincss1.html' because its MIME type was invalid.
> > > +CONSOLE MESSAGE: line 9: Did not parse stylesheet at
> > > 'http://127.0.0.1:8000/resources/redirect.php?url=http://localhost:8000/
> > > security/resources/xorigincss1.html' because its MIME type was invalid.
> > > 
> > > I thought that the inconsistent line number detection was fixed for good,
> > > but apparently not :(
> > 
> > :'( I'll see if I can reproduce.
> 
> I cannot reproduce. However, it looks like the relevant code is in
> getParserLocationForConsoleMessage() from PageConsoleClient.cpp. It looks
> like a line number is computed if we're still parsing the HTML document when
> we receive the CSS resource and try to parse it.
> 
> For the CSS to get loaded while we're still parsing the link element, it has
> to mean that the CSS resource was in the memory cache when requested. As a
> result, my proposal would be to clear the memory cache using
> internals.clearMemoryCache() at the beginning of these tests.

Actually, if I call "internals.clearMemoryCache();" at the beginning of these tests, the line number then shows up in the console messages for me locally. So it seems I am misunderstanding something.

[Chris Dumez]

Reopening due to flaky tests.

[Chris Dumez]

Created attachment 250906 [details]
Patch

[Chris Dumez]

(In reply to comment #20)
> Created attachment 250906 [details]
> Patch

Proposing a patch which gets rid of the line numbers in the CONSOLE message entirely.
I don't think the line number is very useful in this case anyway and the stylesheet URL is provided and the issue is with its MIME type.

[WebKit Commit Bot]

Comment on attachment 250906 [details]
Patch

Clearing flags on attachment: 250906

Committed r182881: <http://trac.webkit.org/changeset/182881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #22)
> Comment on attachment 250906 [details]
> Patch
> 
> Clearing flags on attachment: 250906
> 
> Committed r182881: <http://trac.webkit.org/changeset/182881>

It broke the Apple Windows build as the EWS signaled.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #22)
> > Comment on attachment 250906 [details]
> > Patch
> > 
> > Clearing flags on attachment: 250906
> > 
> > Committed r182881: <http://trac.webkit.org/changeset/182881>
> 
> It broke the Apple Windows build as the EWS signaled.

Well, EWS signaled a bit late. <Fixed in https://trac.webkit.org/r182887>.