Bug 85302 – Add support for seamless attribute as well as seamless sandbox flag and default CSS styling

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add support for seamless attribute as well as seamless sandbox flag and default CSS styling

[Eric Seidel (no email)]

Created attachment 139662 [details]
Patch

[Ojan Vafai]

Comment on attachment 139662 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        were still to have this seamles styling.  I've added additional testing for this case.

typo: seamles

> Source/WebCore/html/HTMLIFrameElement.cpp:120
> +    return hasAttribute(seamlessAttr) && contentDocument() && contentDocument()->mayDisplaySeamlessWithParent();

Doubt it matters, but you might want to put the hasAttribute check last since it's slower than the other two null-checks just to be safe.

[Adam Barth]

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

> Source/WebCore/dom/SecurityContext.h:70
> +    bool mayDisplaySeamlessWithParent() const { return m_mayDisplaySeamlessWithParent; }

You might also consider always returning false here until you're ready to rock and roll with seamless.

> Source/WebCore/html/HTMLIFrameElement.idl:33
> +        attribute [Reflect] boolean seamless;

You might consider adding this attribute last since this is what folks will use to detect whether this feature is present.

[Eric Seidel (no email)]

Comment on attachment 139662 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        were still to have this seamles styling.  I've added additional testing for this case.
> 
> typo: seamles

Fixed.

>> Source/WebCore/html/HTMLIFrameElement.cpp:120
>> +    return hasAttribute(seamlessAttr) && contentDocument() && contentDocument()->mayDisplaySeamlessWithParent();
> 
> Doubt it matters, but you might want to put the hasAttribute check last since it's slower than the other two null-checks just to be safe.

OK, done.

[Eric Seidel (no email)]

(In reply to comment #3)
> View in context: https://bugs.webkit.org/attachment.cgi?id=139662&action=review
> 
> > Source/WebCore/dom/SecurityContext.h:70
> > +    bool mayDisplaySeamlessWithParent() const { return m_mayDisplaySeamlessWithParent; }
> 
> You might also consider always returning false here until you're ready to rock and roll with seamless.

That would make testing impossible unfortunately. :)

> > Source/WebCore/html/HTMLIFrameElement.idl:33
> > +        attribute [Reflect] boolean seamless;
> 
> You might consider adding this attribute last since this is what folks will use to detect whether this feature is present.

Sure.  I'll leave that out.  It will make some tests fail, but won't affect the rest of the patches.

[WebKit Review Bot]

Comment on attachment 139662 [details]
Patch

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

New failing tests:
svg/in-html/by-reference.html

[WebKit Review Bot]

Created attachment 139675 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Created attachment 139677 [details]
Patch for landing

[Eric Seidel (no email)]

--- /tmp/layout-test-results/http/tests/security/sandboxed-iframe-modify-self-expected.txt 
+++ /tmp/layout-test-results/http/tests/security/sandboxed-iframe-modify-self-actual.txt 
@@ -1,3 +1,5 @@
+CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-form-top.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-form-top.html.
+
 CONSOLE MESSAGE: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-modify-self.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-modify-self.html. Domains, protocols and ports must match.
 
 This is a "sanity" test case to verify that a sandboxed frame cannot break out of its sandbox by modifying its own sandbox attribute. Two attempts are made:

Dr. Barth: does that ring any bells for you?

[Eric Seidel (no email)]

Maybe I just didn't update the http expected files before uploading the first patch. :)  We'll see how the CQ handles my updated one.

[Eric Seidel (no email)]

Although that looks like it might be output bleed from the previous test?

[WebKit Review Bot]

Comment on attachment 139677 [details]
Patch for landing

Rejecting attachment 139677 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
sts/fast/frames/seamless/seamless-inherited-origin.html
patching file LayoutTests/fast/frames/seamless/seamless-inline-expected.txt
patching file LayoutTests/fast/frames/seamless/seamless-min-max-expected.txt
patching file LayoutTests/fast/frames/seamless/seamless-sandbox-flag-expected.txt
patching file LayoutTests/fast/frames/seamless/seamless-sandbox-flag.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12597516

[Eric Seidel (no email)]

Created attachment 139683 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139683 [details]
Patch for landing

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

New failing tests:
svg/in-html/by-reference.html

[WebKit Review Bot]

Created attachment 139697 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Crazy.  svg/in-html/by-reference.html uses "seamless".  It doesn't want to though, so I'll remove the mention.

[Eric Seidel (no email)]

Created attachment 139698 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139698 [details]
Patch for landing

Clearing flags on attachment: 139698

Committed r115773: <http://trac.webkit.org/changeset/115773>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Comment on attachment 139698 [details]
Patch for landing

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

> Source/WebCore/css/html.css:986
> +iframe:not([seamless]) {

This just occurred to me... :not is super slow. I think :not of an attribute selector might be especially slow. Might want to make sure this doesn't regress performance on pages that do or don't have seamless iframes.

[Ojan Vafai]

Antti might have thoughts on whether it's OK performance-wise to use iframe:not[seamless] in html.css.

[Eric Seidel (no email)]

The bots will have thoughts. :)  Until then, I'm not worried.

[Antti Koivisto]

iframes are not that frequent and :not is not that bad. Should be ok.

(In reply to comment #22)
> The bots will have thoughts. :)  Until then, I'm not worried.

That's not a good way to think about the performance implications of patches. The bot coverage is far from perfect.

[Eric Seidel (no email)]

Clearly we should make the bots better then. :)

As Knuth said, "Premature optimization is the root of all evil."  I refuse to be afraid. :)

Thank you both for your thoughts.

[Ojan Vafai]

(In reply to comment #24)
> Clearly we should make the bots better then. :)
> 
> As Knuth said, "Premature optimization is the root of all evil."  I refuse to be afraid. :)
> 
> Thank you both for your thoughts.

There's being afraid and there's thinking about your code. Surely you wouldn't add an n-factorial algorithm in performance critical code that happens to not be covered by the bots (to take an extreme case). I don't think we can make a blanket rule that you don't need to think about performance if the bots don't cover that code.

I'd be OK with saying this if your patch came with a performance test for the code in question.

[Eric Seidel (no email)]

(In reply to comment #25)
> (In reply to comment #24)
> > Clearly we should make the bots better then. :)
> > 
> > As Knuth said, "Premature optimization is the root of all evil."  I refuse to be afraid. :)
> > 
> > Thank you both for your thoughts.
> 
> There's being afraid and there's thinking about your code. 

And with that, you assume I don't think about my code. :)  Surely you don't mean to sound that way. :)

> I'd be OK with saying this if your patch came with a performance test for the code in question.

And that, my friend, would be again, FUD, and premature optimization.  Why should we assume that our existing perf tests do not cover this case?  How about the 1000 other changes which we check in weekly, for which you or I or anitti didn't think about the perf implications?  There is no way any of us are a better predictor than the actual tests themselves. :)

[Eric Seidel (no email)]

I look forward to discussing the more interesting aspects of seamless with you both.  There are many tricky places in the code, but this patch (and the previous one where I only added tests!) is certainly not it. :)