Bug 85914 - Add stylesheet inheritance support to IFRAME_SEAMLESS
Summary: Add stylesheet inheritance support to IFRAME_SEAMLESS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 45950
  Show dependency treegraph
 
Reported: 2012-05-08 14:10 PDT by Eric Seidel (no email)
Modified: 2012-05-16 15:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.74 KB, patch)
2012-05-08 14:15 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (1.83 MB, application/zip)
2012-05-08 15:39 PDT, WebKit Review Bot
no flags Details
Patch for landing (8.81 KB, patch)
2012-05-08 16:26 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
reduced performance test (114.46 KB, text/html)
2012-05-15 15:29 PDT, Ojan Vafai
no flags Details
results from running ojan's reduced test on my machine (7.11 KB, text/plain)
2012-05-15 17:06 PDT, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-05-08 14:10:33 PDT
Add stylesheet inheritance support to IFRAME_SEAMLESS
Comment 1 Eric Seidel (no email) 2012-05-08 14:15:30 PDT
Created attachment 140782 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-08 15:39:23 PDT
Comment on attachment 140782 [details]
Patch

Attachment 140782 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12653396

New failing tests:
http/tests/security/contentSecurityPolicy/xsl-blocked.php
http/tests/security/xss-DENIED-xsl-external-entity.xml
accessibility/aria-disabled.html
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
fast/frames/lots-of-objects.html
fast/parser/external-entities.xml
compositing/sibling-positioning.html
fast/loader/text-document-wrapping.html
fast/events/zoom-dblclick.html
http/tests/security/cookies/first-party-cookie-allow-xslt.xml
fast/xsl/extra-lf-at-end.html
fast/canvas/webgl/shader-precision-format.html
http/tests/security/contentSecurityPolicy/xsl-unaffected-by-style-src-1.php
fast/xsl/sort-unicode.xml
fast/xsl/dtd-in-source-document.xml
http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml
http/tests/misc/location-with-space.php
fast/xsl/nbsp-in-stylesheet.html
http/tests/security/cookies/third-party-cookie-blocking-xslt.xml
http/tests/security/cookies/assign-document-url.html
fast/xsl/import-non-document-node.xhtml
fast/performance/performance-now-timestamps.html
Comment 3 WebKit Review Bot 2012-05-08 15:39:30 PDT
Created attachment 140801 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Eric Seidel (no email) 2012-05-08 15:40:18 PDT
Odd.  I'll test again locally.
Comment 5 Eric Seidel (no email) 2012-05-08 15:42:37 PDT
It appears I'm missing a null check.  Investigating.
Comment 6 Ojan Vafai 2012-05-08 15:49:14 PDT
Comment on attachment 140782 [details]
Patch

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

This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either.

1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine.
2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this.

"In a CSS-supporting user agent: the user agent must add all the style sheets that apply to the iframe element to the cascade of the active document of the iframe element's nested browsing context, at the appropriate cascade levels, before any style sheets specified by the document itself.

In a CSS-supporting user agent: the user agent must, for the purpose of CSS property inheritance only, treat the root element of the active document of the iframe element's nested browsing context as being a child of the iframe element. (Thus inherited properties on the root element of the document in the iframe will inherit the computed values of those properties on the iframe element instead of taking their initial values.)"

Other than that, the patch looks good to me.

> Source/WebCore/css/StyleResolver.cpp:433
> +    HTMLFrameOwnerElement* ownerElement = document()->ownerElement();
> +    Vector<StyleSheetList*> ancestorSheets;
> +    while (ownerElement && ownerElement->hasTagName(iframeTag) && static_cast<HTMLIFrameElement*>(ownerElement)->shouldDisplaySeamlessly()) {
> +        ancestorSheets.append(ownerElement->document()->styleSheets());
> +        ownerElement = ownerElement->document()->ownerElement();

I'd rather this code use seamlessParentIFrame to get the ownerElement. Seems like it would at least simplify the while clause.
Comment 7 Ojan Vafai 2012-05-08 15:49:49 PDT
Antti, mind taking a quick look at this to make sure it's sane?
Comment 8 Eric Seidel (no email) 2012-05-08 15:59:14 PDT
(In reply to comment #6)
> (From update of attachment 140782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review
> 
> This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either.
> 
> 1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine.

Interesting.  I'm not sure how that differs from what I did.  My interpretation of that is that any sheet which "could" apply to the iframe, should be included.  Even if that sheet doesn't affect the style of the iframe due to it's selector choices.  Are you suggesting that I'm including sheets which may not match the media queries of the child or some such?

> 2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this.

Yes.  There are lots of tests for this, but that's the next change.  I intentionally made that separate from this one, as it's slightly more controversial in implementation due to details of how we use a Style on the Document, instead of inheriting directly to the DocumentElement (like the spec would imply).  I"ll clarify this all in my next patch.

> "In a CSS-supporting user agent: the user agent must add all the style sheets that apply to the iframe element to the cascade of the active document of the iframe element's nested browsing context, at the appropriate cascade levels, before any style sheets specified by the document itself.

Yes, that's the line this patch intends to implement.

> In a CSS-supporting user agent: the user agent must, for the purpose of CSS property inheritance only, treat the root element of the active document of the iframe element's nested browsing context as being a child of the iframe element. (Thus inherited properties on the root element of the document in the iframe will inherit the computed values of those properties on the iframe element instead of taking their initial values.)"

Yup.  That's the very next patch!

> Other than that, the patch looks good to me.
> 
> > Source/WebCore/css/StyleResolver.cpp:433
> > +    HTMLFrameOwnerElement* ownerElement = document()->ownerElement();
> > +    Vector<StyleSheetList*> ancestorSheets;
> > +    while (ownerElement && ownerElement->hasTagName(iframeTag) && static_cast<HTMLIFrameElement*>(ownerElement)->shouldDisplaySeamlessly()) {
> > +        ancestorSheets.append(ownerElement->document()->styleSheets());
> > +        ownerElement = ownerElement->document()->ownerElement();
> 
> I'd rather this code use seamlessParentIFrame to get the ownerElement. Seems like it would at least simplify the while clause.

Ah yes!  Sorry, that method was added later.  I'll update the patch shortly.  Thank you very much for the review!
Comment 9 Eric Seidel (no email) 2012-05-08 16:02:30 PDT
Yes, I didn't consider the possibility that the Document had not yet been inserted into a Frame when it got a stylesheet update.  I've added the appropriate null check to the next patch.
Comment 10 Eric Seidel (no email) 2012-05-08 16:26:34 PDT
Created attachment 140809 [details]
Patch for landing
Comment 11 Ojan Vafai 2012-05-08 17:09:55 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 140782 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review
> > 
> > This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either.
> > 
> > 1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine.
> 
> Interesting.  I'm not sure how that differs from what I did.  My interpretation of that is that any sheet which "could" apply to the iframe, should be included.  Even if that sheet doesn't affect the style of the iframe due to it's selector choices.  Are you suggesting that I'm including sheets which may not match the media queries of the child or some such?

nm. I was confused.

> > 2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this.
> 
> Yes.  There are lots of tests for this, but that's the next change.  I intentionally made that separate from this one, as it's slightly more controversial in implementation due to details of how we use a Style on the Document, instead of inheriting directly to the DocumentElement (like the spec would imply).  I"ll clarify this all in my next patch.

Sounds good. I didn't see it on github, but I was probably just looking for the wrong thing.
Comment 12 Ojan Vafai 2012-05-08 17:11:35 PDT
Comment on attachment 140809 [details]
Patch for landing

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

> Source/WebCore/css/StyleResolver.cpp:434
> +        Document* parentDocument = parentIFrame->document();
> +        ancestorSheets.append(parentDocument->styleSheets());
> +        childDocument = parentDocument;

Nit: can't this just be:
ancestorSheets.append(parentDocument->styleSheets());
childDocument = parentIFrame->document();
Comment 13 Eric Seidel (no email) 2012-05-08 17:13:26 PDT
Comment on attachment 140809 [details]
Patch for landing

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

>> Source/WebCore/css/StyleResolver.cpp:434
>> +        childDocument = parentDocument;
> 
> Nit: can't this just be:
> ancestorSheets.append(parentDocument->styleSheets());
> childDocument = parentIFrame->document();

I assume you mean ancestorSheets.append(parentIframe->document()->styleSheets());  with the goal being to remove the parentDocument local?
Comment 14 Eric Seidel (no email) 2012-05-08 17:13:52 PDT
Thanks again for the reviews!  I hope to post the iframe-style-inheritance patch shortly, but we can go over that tomorrow. :)
Comment 15 WebKit Review Bot 2012-05-08 17:51:24 PDT
Comment on attachment 140809 [details]
Patch for landing

Clearing flags on attachment: 140809

Committed r116471: <http://trac.webkit.org/changeset/116471>
Comment 16 WebKit Review Bot 2012-05-08 17:51:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Eric Seidel (no email) 2012-05-12 10:56:56 PDT
It's not particularly clear what the test is trying to do:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html

This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive.

But Ill investigate on monday.  Thank you for the note.
Comment 19 Eric Seidel (no email) 2012-05-14 15:23:47 PDT
(In reply to comment #18)
> It's not particularly clear what the test is trying to do:
> http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html
> 
> This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive.
> 
> But Ill investigate on monday.  Thank you for the note.

That test appears to be *super* noisy on my machine.

It seems to load the entire dromero test suite in an iframe and then sucks out the number it cares about. :(

There is nothign for me to do here of use. :(
Comment 20 Ryosuke Niwa 2012-05-14 15:58:14 PDT
Yeah, I profiled DRT but nothing came out. It's probably caused by some memory layout / compiler optimization changes. I agree we can't do anything here.
Comment 21 Ojan Vafai 2012-05-14 16:34:07 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > It's not particularly clear what the test is trying to do:
> > http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html
> > 
> > This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive.
> > 
> > But Ill investigate on monday.  Thank you for the note.
> 
> That test appears to be *super* noisy on my machine.
> 
> It seems to load the entire dromero test suite in an iframe and then sucks out the number it cares about. :(
> 
> There is nothign for me to do here of use. :(

Seems like we should either try a speculative patch to verify the theory or rollback while you figure out what the issue is. It's a clear 6% regression and, assuming the perf dashboard is accurate, it's clearly from this patch.

It sucks that the test is too noisy for local debugging, but I think that just means you need to stare at the code and try speculative changes and see how they come out on the bots.

One easy way to verify your theory that the issue is in StyleResolver would be to ifdef out the suspicious lines from addStylesheetsFromSeamlessParents. As in, is it just the seamlessParentIFrame call? Seems really unlikely to me since ENABLE_IFRAME_SEAMLESS is false.
Comment 22 Ojan Vafai 2012-05-14 16:36:33 PDT
Alternatley, you could try tweaking the test locally to see if you can exaggerate the difference. For example, you could comment out all the "test" calls except 1 and try each test call separately to see which one regressed. You could also increase "num" to try and reduce the variance. I imagine that combining those two you ought to be able to get a reasonably reduced perf test case.
Comment 23 Eric Seidel (no email) 2012-05-15 14:44:56 PDT
On my (mostly quieted) machine, I see the following output from dom-attr:

1780
1545.6
392.6500645063519
521.0957042957043
327.27105788423154
434.824975024975

avg 5001.441801711263 runs/s
median 0 runs/s
stdev 117.64701441386933 runs/s
min 4856.96368102955 runs/s
max 5131.251536886268 runs/s

I'm assuming those first 6 numbers are not run results (as they are for other perf tests) as they do not corrolate with the min/max values, and if tehy were, would lead me to believe the test was amazingly noisy.

Successive runs only got slower:

1686.4
1480.2
386.69210789210786
509.89390609390614
321.2131736526946
428.4850562611042

avg 4812.884243899814 runs/s
median 0 runs/s
stdev 98.9989274854694 runs/s
min 4691.776064255106 runs/s
max 4988.916731970625 runs/s


1614.4
1482.4
389.28866702160116
508.89490509490514
329.60252880851687
438.5374625374625

avg 4763.123563462485 runs/s
median 0 runs/s
stdev 100.25685680375 runs/s
min 4650.014642044582 runs/s
max 4925.60939060939 runs/s
Comment 24 Eric Seidel (no email) 2012-05-15 15:25:51 PDT
The entire test is adding/removing attributes from dom objects:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html#L5

As far as I can tell, dromero is running the tests each 5 times:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/webrunner.js#L7

The progress reports, are the current "mean" value from Dromero:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeorunner.js#L58

Which are not comparable with the end stats, since the end stats are the sum of the means:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeorunner.js#L5

The only one of these subtests which could have anything to do with style, is the id changes.  Which will trigger an updateId call and a setNeedsStyleRecalc()
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L2011
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.h#L582
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L706

However, the test never triggers a style recalc, so this should have no effect.

The only effect from my change is that StyleResolver will take slightly longer to construct.  However, the only StyleResolvers being constructed here are the ones when the initial documents (for this test) are created.  There are no style updates happenign which would cause the StyleResolver to recalc.


When profiling in Instruments, this test is largely dominated by jsString and AtomicString creation.  This test (intentionally?) is thrashing the string tables as hard as it can. :)  Again, nothing to do with style resolution or my change.

I expect that what we're seeing here, is compiler noise, per rniwa.  Our scores on this test are so astronomically high as to be useless.  To suggest that the entire test runs in 1/5000th of a second?  And that my change made it so it runs 5% slower than that is meaningless as far as I can tell.

It appears it is litterally calling the testing function 5000 times in the second:
http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/webrunner.js#L85

Wow.  :)  So yes, we run this test incredibly fast.  I'm not surprised that our speed here would vary by *huge* amounts depending on how the compiler is feeling that day.  Similarly, I'm not surprised this test is super noisy.

Unless you have further data, I believe we should ignore this "regression" and in general ignore any changes in dom-attr results in the future, unless they're incredibly large. :)
Comment 25 Ojan Vafai 2012-05-15 15:29:16 PDT
Created attachment 142080 [details]
reduced performance test

This test reliably shows a 5-6% regression from this patch. It logs the output to the console:

r116448:
522 dom-attr.html:19
518 dom-attr.html:19
516 dom-attr.html:19
528 dom-attr.html:19
517 dom-attr.html:19
518 dom-attr.html:19
522 dom-attr.html:19
517 dom-attr.html:19
517 dom-attr.html:19
525 dom-attr.html:19
Avg:520 

r116491:
553 dom-attr.html:19
548 dom-attr.html:19
549 dom-attr.html:19
546 dom-attr.html:19
547 dom-attr.html:19
555 dom-attr.html:19
547 dom-attr.html:19
544 dom-attr.html:19
550 dom-attr.html:19
547 dom-attr.html:19
Avg:548.6 

Sometimes the first load is a little noisy, but reloading the page consistently shows the regression. Looks to be that setting IDs has gotten slower.
Comment 26 Eric Seidel (no email) 2012-05-15 15:57:30 PDT
In the spirit of further investigation, I ran dom-attr.html in the debugger on tip of tree.

I placed a break point at the callsites for the two functions I added:
notifySeamlessChildDocumentsOfStylesheetUpdate
and
addStylesheetsFromSeamlessParents

They were hit about 8 times total during page load (as expected).  But then never during the actual running of the test.

I don't have any explanation as to why this test result changes after my patch beyond that fact that the test  appears so tight as to be testing noise, and that my change happens to be unlucky. :(

I've also grepped the Dromero source to make sure that nowhere does it use the seamless attribute.

I then ran the test again in the debugger, this time with breakpoints on the seamless code paths (where we actually add stylesheets from the parent docuemnts, etc.) to make sure there was no way the test was going down those paths (as my grep would suggest to be impossible).

And correctly verified that the debugger never broke.

I'm out of ideas.  But as stated in comment #24, I believe this microbenchmark to be testing noise at this point, and thus this "regresssion" is meaningless.  Comment #17 would suggest that Ryosuke agrees.
Comment 27 Eric Seidel (no email) 2012-05-15 16:18:46 PDT
(In reply to comment #25)
> Created an attachment (id=142080) [details]
> reduced performance test
> 
> This test reliably shows a 5-6% regression from this patch. It logs the output to the console:
> 
> r116448:
> 522 dom-attr.html:19
> 518 dom-attr.html:19
> 516 dom-attr.html:19
> 528 dom-attr.html:19
> 517 dom-attr.html:19
> 518 dom-attr.html:19
> 522 dom-attr.html:19
> 517 dom-attr.html:19
> 517 dom-attr.html:19
> 525 dom-attr.html:19
> Avg:520 
> 
> r116491:
> 553 dom-attr.html:19
> 548 dom-attr.html:19
> 549 dom-attr.html:19
> 546 dom-attr.html:19
> 547 dom-attr.html:19
> 555 dom-attr.html:19
> 547 dom-attr.html:19
> 544 dom-attr.html:19
> 550 dom-attr.html:19
> 547 dom-attr.html:19
> Avg:548.6 
> 
> Sometimes the first load is a little noisy, but reloading the page consistently shows the regression. Looks to be that setting IDs has gotten slower.

I'm curious how you generated these numbers?

Using run-perf-tests I get a different output:
run-perf-tests PerformanceTests/Dromaeo/dom-attr.html --release                                                                                                                                                                                                                                         [/Projects/WebKit]
Running Dromaeo/dom-attr.html (1 of 1)
RESULT Dromaeo: dom-attr= 4829.11909288 runs/s
median= 0.0 runs/s, stdev= 110.628063799 runs/s, min= 4669.4222185 runs/s, max= 4975.24575425 runs/s

Was this from the console log on your machine?
Comment 28 Ojan Vafai 2012-05-15 16:21:46 PDT
(In reply to comment #27)
> I'm curious how you generated these numbers?
> 
> Using run-perf-tests I get a different output:
> run-perf-tests PerformanceTests/Dromaeo/dom-attr.html --release                                                                                                                                                                                                                                         [/Projects/WebKit]
> Running Dromaeo/dom-attr.html (1 of 1)
> RESULT Dromaeo: dom-attr= 4829.11909288 runs/s
> median= 0.0 runs/s, stdev= 110.628063799 runs/s, min= 4669.4222185 runs/s, max= 4975.24575425 runs/s
> 
> Was this from the console log on your machine?

This was from the console log on my machine running the test case I attached.
Comment 29 Eric Seidel (no email) 2012-05-15 17:06:06 PDT
Created attachment 142098 [details]
results from running ojan's reduced test on my machine

Am I reading this correctly?  It looks like my numbers show the seamless change as a 10% speed-up on your benchmark?
(assuming smaller numbers are better, which appears to be the case from your use of "time").
Comment 30 Ojan Vafai 2012-05-15 17:46:52 PDT
(In reply to comment #29)
> Created an attachment (id=142098) [details]
> results from running ojan's reduced test on my machine
> 
> Am I reading this correctly?  It looks like my numbers show the seamless change as a 10% speed-up on your benchmark?
> (assuming smaller numbers are better, which appears to be the case from your use of "time").

Strange. Those are not the numbers I get running at the same revisions. I'm testing on chromium-linux FWIW. In either case, I'm increasingly convinced that the regression is not from this revision and that perf-o-matic was just wrong. I'm binary searching through revisions to find the culprit.
Comment 31 Ryosuke Niwa 2012-05-15 18:12:51 PDT
I don't think we should spend any more time investigating this issue. While it is possible that this patch caused a performance regression, the cause of the regression is beyond our control from what I can tell. It could be memory layout being different, some compiler optimization not working, or other odd source of perf. hits. Whatever it is, there isn't much we can do at this point.

It's probably more productive to spend time on how to improve the score on that test than trying to figure out what the regression in this case.
Comment 32 Eric Seidel (no email) 2012-05-15 23:28:48 PDT
From my instruments runs I suspect we could improve these numbers by looking closely at when we're creating atomic strings and seeing if we could avoid them. :)  That said, this benchmark is doing the same thing 5 MILLLION times in .5 seconds, and thus even if we did improve it, is unlikely to have any effect on real page loads.  I recommend we ignore/disable the benchmark in the future. :)
Comment 33 Ojan Vafai 2012-05-16 15:10:35 PDT
The perf regression was caused by http://trac.webkit.org/changeset/116487 and is being fixed in https://bugs.webkit.org/show_bug.cgi?id=86680. We looked, and perf-o-matic is getting data that matches the buildbot waterfall. So, either the bot is confused about what revision's build it's running, or we just got unlucky and the test high test variance just made it really seem like the seamless patch was the culprit.