Bug 102110 - REGRESSION(r129644): User StyleSheet not applying
Summary: REGRESSION(r129644): User StyleSheet not applying
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Tony Chang
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2012-11-13 11:39 PST by Tony Chang
Modified: 2012-11-20 14:09 PST (History)
11 users (show)

See Also:


Attachments
Patch (test only) (2.53 KB, patch)
2012-11-13 11:40 PST, Tony Chang
no flags Details | Formatted Diff | Diff
patch (20.19 KB, patch)
2012-11-16 11:39 PST, Antti Koivisto
kling: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
updated patch (18.59 KB, patch)
2012-11-18 12:17 PST, Antti Koivisto
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
retry (21.20 KB, patch)
2012-11-18 14:13 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-11-13 11:39:02 PST
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
Comment 1 Tony Chang 2012-11-13 11:40:01 PST
Created attachment 173933 [details]
Patch (test only)
Comment 2 Tony Chang 2012-11-13 11:40:55 PST
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.
Comment 3 Tony Chang 2012-11-14 15:41:32 PST
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.
Comment 4 Tony Chang 2012-11-15 16:06:37 PST
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.
Comment 5 Tony Chang 2012-11-15 17:42:03 PST
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?
Comment 6 Antti Koivisto 2012-11-16 11:39:49 PST
Created attachment 174731 [details]
patch
Comment 7 Andreas Kling 2012-11-16 12:02:15 PST
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!
Comment 8 Antti Koivisto 2012-11-16 13:23:08 PST
http://trac.webkit.org/changeset/134986
Comment 9 Dimitri Glazkov (Google) 2012-11-16 13:31:46 PST
Reverted r134986 for reason:

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

Committed r134992: <http://trac.webkit.org/changeset/134992>
Comment 10 Build Bot 2012-11-17 06:00:21 PST
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
Comment 11 Antti Koivisto 2012-11-18 12:17:42 PST
Created attachment 174852 [details]
updated patch

Removed a faulty ASSERT and updated one test result.
Comment 12 WebKit Review Bot 2012-11-18 14:03:01 PST
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
Comment 13 Antti Koivisto 2012-11-18 14:13:08 PST
Created attachment 174856 [details]
retry

forgot the test update
Comment 14 WebKit Review Bot 2012-11-18 14:39:33 PST
Comment on attachment 174856 [details]
retry

Clearing flags on attachment: 174856

Committed r135082: <http://trac.webkit.org/changeset/135082>
Comment 15 WebKit Review Bot 2012-11-18 14:39:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Antti Koivisto 2012-11-19 04:12:06 PST
A WebKit test case would be cool.
Comment 18 Kentaro Hara 2012-11-19 04:13:50 PST
Yeah, I'm trying to invent a WebKit test case to reproduce the bug (but still not successful:-).
Comment 19 Jian Li 2012-11-19 14:07:30 PST
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
Comment 20 Tony Chang 2012-11-20 14:09:04 PST
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.