Bug 102110 – REGRESSION(r129644): User StyleSheet not applying

• Component: CSS
• Status: RESOLVED FIXED

[Tony Chang]

I was able to bisect this to http://trac.webkit.org/changeset/129644 . I'll upload a layout test that repros the problem.

Antti, can you take a look?

This was originally reported to Chromium: https://code.google.com/p/chromium/issues/detail?id=160263

[Tony Chang]

Created attachment 173933 [details]
Patch (test only)

[Tony Chang]

Comment on attachment 173933 [details]
Patch (test only)

The user stylesheet should apply and you should see a green box with "PASS" in it.  If you see a red box, the test failed.

[Tony Chang]

Hmm, looks like neither Antti nor I can repro this failure on Apple DRT on Mac.

I'm surprised that there's differing behavior here (maybe a timing issue in the network layer?)  I'll see if I can figure out why the ports don't match.

[Tony Chang]

Ok, I figured out the difference.  In Chromium, when an extension adds a style sheet, we add it as an author level style sheet.  In Safari, when an extension adds a style sheet, we add it as a user level style sheet.

I'm still trying to figure out why this difference doesn't work with Antti's change.

[Tony Chang]

I think what's happening is:

In http://trac.webkit.org/browser/trunk/Source/WebCore/dom/DocumentStyleSheetCollection.cpp#L470 , we call resetAuthorStyle(), which clears all author styles, including those that were added from pageGroupUserSheets here: http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L321.

Maybe in resetAuthorStyle we should add a loop over styleSheetCollection->pageGroupUserSheets() that adds back the extension specified author stylesheets?

[Antti Koivisto]

Created attachment 174731 [details]
patch

[Andreas Kling]

Comment on attachment 174731 [details]
patch

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

r=me..

> Source/WebCore/css/StyleResolver.cpp:346
> +    for (unsigned i = 0; i < userSheets.size(); i++) {

++i

> Source/WebCore/css/StyleResolver.cpp:2583
> +    for (size_t i = 0; i < sheets.size(); ++i)

size_t!

> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:82
> +    for (size_t i = 0; i < m_userStyleSheets.size(); ++i)

size_t again!

[Antti Koivisto]

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

[Dimitri Glazkov (Google)]

Reverted r134986 for reason:

Triggered ASSERT in fast/frames/seamless/seamless-inherited-origin.html.

Committed r134992: <http://trac.webkit.org/changeset/134992>

[Build Bot]

Comment on attachment 174731 [details]
patch

Attachment 174731 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14875335

New failing tests:
inspector/timeline/timeline-script-tag-1.html

[Antti Koivisto]

Created attachment 174852 [details]
updated patch

Removed a faulty ASSERT and updated one test result.

[WebKit Review Bot]

Comment on attachment 174852 [details]
updated patch

Rejecting attachment 174852 [details] from commit-queue.

New failing tests:
inspector/timeline/timeline-script-tag-1.html
Full output: http://queues.webkit.org/results/14888544

[Antti Koivisto]

Created attachment 174856 [details]
retry

forgot the test update

[WebKit Review Bot]

Comment on attachment 174856 [details]
retry

Clearing flags on attachment: 174856

Committed r135082: <http://trac.webkit.org/changeset/135082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

This patch breaks browser_tests in Chromium:

log: http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/24821/steps/browser_tests/logs/ContentScriptCSSLocalization

test: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/extensions/api_test/content_scripts/css_l10n/background.js&exact_package=chromium&q=textDirectionMessageGetsReplacedInInsertCSSCall&type=cs&l=48

[Antti Koivisto]

A WebKit test case would be cool.

[Kentaro Hara]

Yeah, I'm trying to invent a WebKit test case to reproduce the bug (but still not successful:-).

[Jian Li]

Got an assert from one Chromium build bot:

ASSERTION FAILED: userSheets[i]->contents()->isUserStyleSheet()
third_party/WebKit/Source/WebCore/css/StyleResolver.cpp(340) : void WebCore::StyleResolver::collectRulesFromUserStyleSheets(const WTF::Vector<WTF::RefPtr<WebCore::CSSStyleSheet>, 0u>&, WebCore::RuleSet&)

Here is the link to bot output:

http://build.chromium.org/p/chromium.linux/builders/Linux%20%28Precise%29/builds/651/steps/browser_tests/logs/ExecuteScriptBasic

[Tony Chang]

There are 2 bugs, both in Chromium:

1) We were calling DocumentStyleSheetCollection::addUserSheet and trying to add an author level stylesheet.  This is chromium using the DocumentStyleSheetCollection incorrectly and I'm fixing it in bug 102835.  This will fix the asserts in ExecuteScriptApiTest.ExecuteScriptBasic, ExecuteScriptApiTest.ExecuteScriptInFrame.

2) Because we were adding an author level style sheet via DocumentStyleSheetCollection::addUserSheet, it was getting matched by window.getMatchedCSSRules (which only matches author level stylesheets).  This this is Chromium specific, I filed a bug at http://crbug.com/162096 for this.  It's not clear to me if we want to just live with the current behavior or add functionality to addAuthorSheets.