Bug 103255 – Log to console when autofocus is blocked by sandbox attribute.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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.

[Mike West]

Created attachment 176003 [details]
Patch

[Ojan Vafai]

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.

[Ojan Vafai]

Perhaps this and bug 102750 could use the same solution.

[Mike West]

(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.

[Ojan Vafai]

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.

[Mike West]

Thanks Ojan. I've filed bug 103274 to continue the conversation about a UI for these sorts of warnings.

[Mike West]

Created attachment 176256 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

I'll let Ojan review this patch since he seems concerned about spamming the console, which is the main consideration here.

[Ojan Vafai]

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.

[Mike West]

(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?

[Mike West]

(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.

[Ojan Vafai]

I don't have a preference here. I defer to Adam. :)

[Mike West]

Created attachment 176313 [details]
Patch

[Mike West]

(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?

[Ojan Vafai]

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

[Mike West]

Created attachment 176317 [details]
Patch for landing

[Mike West]

(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!

[WebKit Review Bot]

Comment on attachment 176317 [details]
Patch for landing

Clearing flags on attachment: 176317

Committed r135903: <http://trac.webkit.org/changeset/135903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.