Bug 103255 - Log to console when autofocus is blocked by sandbox attribute.
Summary: Log to console when autofocus is blocked by sandbox attribute.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 101964
  Show dependency treegraph
 
Reported: 2012-11-26 07:56 PST by Mike West
Modified: 2012-11-27 12:44 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.54 KB, patch)
2012-11-26 07:59 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2012-11-27 06:39 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2012-11-27 11:53 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (4.60 KB, patch)
2012-11-27 12:07 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-11-26 07:56:50 PST
Starting on bug 101964 with a trivial example. If you're ok with the general idea of logging for sandbox violations, then I'll work through the other ~18 call sites in my spare time.
Comment 1 Mike West 2012-11-26 07:59:14 PST
Created attachment 176003 [details]
Patch
Comment 2 Ojan Vafai 2012-11-26 09:47:17 PST
Comment on attachment 176003 [details]
Patch

I'm not opposed to this, but I'd strongly prefer a UI that shows the error in the place it happened (e.g. inline in the html of the page and/or next to the DOM node in the elements panel). It's too easy to end up with a bunch of spam in the console that makes the console warnings useless.
Comment 3 Ojan Vafai 2012-11-26 09:50:51 PST
Perhaps this and bug 102750 could use the same solution.
Comment 4 Mike West 2012-11-26 10:31:03 PST
(In reply to comment #2)
> (From update of attachment 176003 [details])
> I'm not opposed to this, but I'd strongly prefer a UI that shows the error in the place it happened (e.g. inline in the html of the page and/or next to the DOM node in the elements panel). It's too easy to end up with a bunch of spam in the console that makes the console warnings useless.

1. Once bug 100650 lands, this message would have a line number and url associated with it, which means you could jump directly to the place where the error occurred.

2. I agree with the larger point, though. It would be very nice indeed to have a separate location for errors like this, which are conceptually distinct from the general console messages that a developer might throw to herself. Given that that idea has been floating around for quite some time, though, I'd rather err towards giving the developer too much information, rather than just blocking some action and not explaining why. This bug is a poor example, of course. A better sandboxing example would be something more obscure, like a plugin not loading in a sandboxed iframe. Currently, that's a black box.
Comment 5 Ojan Vafai 2012-11-26 10:32:46 PST
I'm OK with moving forward with this patch as is with a FIXME + bug that we should add a UI specifically for audit style warnings so that they don't spam the console.
Comment 6 Mike West 2012-11-26 10:42:15 PST
Thanks Ojan. I've filed bug 103274 to continue the conversation about a UI for these sorts of warnings.
Comment 7 Mike West 2012-11-27 06:39:48 PST
Created attachment 176256 [details]
Patch
Comment 8 Adam Barth 2012-11-27 09:49:09 PST
Comment on attachment 176256 [details]
Patch

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

> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6
> +<iframe sandbox
> +        src="data:text/html,<input autofocus onfocus>"></iframe>

I would have put this in the sandbox test suite.
Comment 9 Adam Barth 2012-11-27 09:49:52 PST
I'll let Ojan review this patch since he seems concerned about spamming the console, which is the main consideration here.
Comment 10 Ojan Vafai 2012-11-27 10:05:37 PST
Comment on attachment 176256 [details]
Patch

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

The test needs a bit of work, but this looks good to me otherwise.

> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:4
> +</script>

Would be good to have a textual description of what this test is testing so people can know if the output is correct.

>> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6
>> +        src="data:text/html,<input autofocus onfocus>"></iframe>
> 
> I would have put this in the sandbox test suite.

And no need to wrap this line.
Comment 11 Mike West 2012-11-27 10:23:26 PST
(In reply to comment #10)
> (From update of attachment 176256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review
> 
> The test needs a bit of work, but this looks good to me otherwise.
> 
> > LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:4
> > +</script>
> 
> Would be good to have a textual description of what this test is testing so people can know if the output is correct.
> 
> >> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6
> >> +        src="data:text/html,<input autofocus onfocus>"></iframe>
> > 
> > I would have put this in the sandbox test suite.
> 
> And no need to wrap this line.

Ok. Just so I'm clear, 'fast/forms/autofocus-in-sandbox.html' is good where it is, as it's really testing autofocus. This test is really testing sandboxing (or a side-effect thereof), so it's better placed along with other sandbox tests. Is that correct?
Comment 12 Mike West 2012-11-27 10:30:56 PST
(In reply to comment #11)
> Ok. Just so I'm clear, 'fast/forms/autofocus-in-sandbox.html' is good where it is, as it's really testing autofocus. This test is really testing sandboxing (or a side-effect thereof), so it's better placed along with other sandbox tests. Is that correct?

'fast/frames/sandboxed-iframe-*' is better, I suppose.
Comment 13 Ojan Vafai 2012-11-27 11:14:50 PST
I don't have a preference here. I defer to Adam. :)
Comment 14 Mike West 2012-11-27 11:53:38 PST
Created attachment 176313 [details]
Patch
Comment 15 Mike West 2012-11-27 11:55:36 PST
(In reply to comment #10)
> (From update of attachment 176256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review
> 
> The test needs a bit of work, but this looks good to me otherwise.

You were right, I rushed it. The latest patch's test is much less of a copout. :)

WDYT?
Comment 16 Ojan Vafai 2012-11-27 12:01:22 PST
Comment on attachment 176313 [details]
Patch

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

> LayoutTests/fast/frames/sandboxed-iframe-autofocus-denied.html:1
> +<!doctype html>

s/doctype/DOCTYPE
Comment 17 Mike West 2012-11-27 12:07:33 PST
Created attachment 176317 [details]
Patch for landing
Comment 18 Mike West 2012-11-27 12:08:04 PST
(In reply to comment #16)
> (From update of attachment 176313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176313&action=review
> 
> > LayoutTests/fast/frames/sandboxed-iframe-autofocus-denied.html:1
> > +<!doctype html>
> 
> s/doctype/DOCTYPE

Done, thanks!
Comment 19 WebKit Review Bot 2012-11-27 12:44:51 PST
Comment on attachment 176317 [details]
Patch for landing

Clearing flags on attachment: 176317

Committed r135903: <http://trac.webkit.org/changeset/135903>
Comment 20 WebKit Review Bot 2012-11-27 12:44:56 PST
All reviewed patches have been landed.  Closing bug.