Bug 85302 - Add support for seamless attribute as well as seamless sandbox flag and default CSS styling
Summary: Add support for seamless attribute as well as seamless sandbox flag and defau...
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-01 12:56 PDT by Eric Seidel (no email)
Modified: 2012-05-02 17:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.53 KB, patch)
2012-05-01 13:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.98 MB, application/zip)
2012-05-01 14:11 PDT, WebKit Review Bot
no flags Details
Patch for landing (20.18 KB, patch)
2012-05-01 14:13 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (19.91 KB, patch)
2012-05-01 14:32 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.05 MB, application/zip)
2012-05-01 16:05 PDT, WebKit Review Bot
no flags Details
Patch for landing (21.62 KB, patch)
2012-05-01 16:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-05-01 12:56:17 PDT
Add support for seamless attribute as well as seamless sandbox flag and default CSS styling
Comment 1 Eric Seidel (no email) 2012-05-01 13:07:26 PDT
Created attachment 139662 [details]
Patch
Comment 2 Ojan Vafai 2012-05-01 13:54:43 PDT
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.
Comment 3 Adam Barth 2012-05-01 13:56:50 PDT
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.
Comment 4 Eric Seidel (no email) 2012-05-01 14:01:56 PDT
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.
Comment 5 Eric Seidel (no email) 2012-05-01 14:02:35 PDT
(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.
Comment 6 WebKit Review Bot 2012-05-01 14:11:00 PDT
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
Comment 7 WebKit Review Bot 2012-05-01 14:11:06 PDT
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
Comment 8 Eric Seidel (no email) 2012-05-01 14:13:16 PDT
Created attachment 139677 [details]
Patch for landing
Comment 9 Eric Seidel (no email) 2012-05-01 14:14:57 PDT
--- /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?
Comment 10 Eric Seidel (no email) 2012-05-01 14:15:30 PDT
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.
Comment 11 Eric Seidel (no email) 2012-05-01 14:16:05 PDT
Although that looks like it might be output bleed from the previous test?
Comment 12 WebKit Review Bot 2012-05-01 14:16:40 PDT
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
Comment 13 Eric Seidel (no email) 2012-05-01 14:32:30 PDT
Created attachment 139683 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-05-01 16:05:42 PDT
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
Comment 15 WebKit Review Bot 2012-05-01 16:05:49 PDT
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
Comment 16 Eric Seidel (no email) 2012-05-01 16:09:10 PDT
Crazy.  svg/in-html/by-reference.html uses "seamless".  It doesn't want to though, so I'll remove the mention.
Comment 17 Eric Seidel (no email) 2012-05-01 16:12:19 PDT
Created attachment 139698 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-05-01 18:09:11 PDT
Comment on attachment 139698 [details]
Patch for landing

Clearing flags on attachment: 139698

Committed r115773: <http://trac.webkit.org/changeset/115773>
Comment 19 WebKit Review Bot 2012-05-01 18:09:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Ojan Vafai 2012-05-01 19:38:23 PDT
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.
Comment 21 Ojan Vafai 2012-05-01 19:39:03 PDT
Antti might have thoughts on whether it's OK performance-wise to use iframe:not[seamless] in html.css.
Comment 22 Eric Seidel (no email) 2012-05-01 19:41:07 PDT
The bots will have thoughts. :)  Until then, I'm not worried.
Comment 23 Antti Koivisto 2012-05-02 03:34:15 PDT
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.
Comment 24 Eric Seidel (no email) 2012-05-02 11:02:12 PDT
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.
Comment 25 Ojan Vafai 2012-05-02 12:02:26 PDT
(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.
Comment 26 Eric Seidel (no email) 2012-05-02 17:46:19 PDT
(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. :)
Comment 27 Eric Seidel (no email) 2012-05-02 17:47:03 PDT
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. :)