Feature detection-friendly restrictions for packaged apps.

Feature detection-friendly restrictions for packaged apps.

Packaged apps do not have access to some features of the Web platform
that are deprecated, or that do not make sense in the context of an
app. Currently, accessing most of those features throws an error.

This change makes thse features behave more like missing featurs on the
Web platform. Whenever possible, accessing restricted features returns
undefined and logs an error to the browser console. Features accessible
via non-configurable ES5 properties (e.g. window.alert, document.write)
either become no-ops or throw errors, on a case-by-case basis.

Deprecated events have their "on*" properties return undefined (as
opposed to null, for events with no handlers) to facilitate feature
detection. Using addEventListener for these events used to throw an
error, and now logs an error to the console and turns into a no-op, to
mimic the behavior for undefined events as well as possible.

This change aims to make JavaScript libraries written for browsers work
in packaged apps as seamlessly as possible, while still providing useful
warnings for developers. This creates a couple of warts: (1) properties
are not removed, so the "in" operator will still see them, and (2)
dispatchEvent will never deliver deprecated events, but it normally
delives un-implemented events.

BUG=330487
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243817

[pwnall-personal, 2013-12-23 22:13:59]

Can you please take a look?

[miket_OOO, 2014-01-07 17:16:52]

Jeffrey, would you mind taking this one? I'm ambivalent about it and need a
stronger opinion.

[pwnall-personal, 2014-01-07 17:50:44]

On 2014/01/07 17:16:52, miket wrote:
> Jeffrey, would you mind taking this one? I'm ambivalent about it and need a
> stronger opinion.

Mike, Jeffrey, here's another argument against booby-trapped properties.

In Blink's layout tests, we use feature detection without try/catch quite often,
to see if we're running in a normal browser or in a test environment. This lets
us write tests that we can run in other browsers, to compare the results. It'd
suck a bit if some other platform decided to booby-trap window.testRunner, for
whatever reason.

https://code.google.com/p/chromium/codesearch#search/&q=%22if%20(window.testR...

In a nutshell, (1) we use feature detection in our codebase quite a bit, and (2)
breaking feature detection sets a bad precedent.

[Jeffrey Yasskin, 2014-01-07 18:11:15]

+Ben too, who's worked on some poisoning for chrome.* APIs whose permissions
aren't granted.

The proposed change sounds like a good idea, because it'll still show an error
in the console, which I think is what we were going for in the first place. I
haven't reviewed the code yet.

[not at google - send to devlin, 2014-01-07 22:58:42]

I agree with the proposal outlined in the CL description.

[Jeffrey Yasskin, 2014-01-08 22:49:17]

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:52: function
disableMethods(object, objectName, methodNames, useThrowingStubs) {
It seems like it would make sense for a *call* to a disabled method to always
throw something, since if the method were absent (undefined), the call would
throw a TypeError. It's only getters that it makes sense to use a console.error
for, right?

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:110: *     is preferred.
What "is preferred."?

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:135: true);
Why does document get throwing method stubs, but window doesn't?

https://codereview.chromium.org/120733003/diff/90001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/restrictions/main.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/restrictions/main.js:112:
window.dispatchEvent(event);
Can you comment how this tests something?

[pwnall-personal, 2014-01-08 23:20:45]

Thank you very much for the feedback, Jeffery! Can you please take another look?

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:52: function
disableMethods(object, objectName, methodNames, useThrowingStubs) {
On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> It seems like it would make sense for a *call* to a disabled method to always
> throw something, since if the method were absent (undefined), the call would
> throw a TypeError. It's only getters that it makes sense to use a
console.error
> for, right?

Completely agreed, if getting the method from the object would return
"undefined". Since we can't do that for non-configurable properties, I don't
think it's OK to throw an exception on call.

I think the following code follows responsible feature detection practices, and
shouldn't break.

if (window.confirm) {
    var response = window.confirm("Are you sure you want to delete this?");
    ...
}

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:110: *     is preferred.
On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> What "is preferred."?

More care when copy-pasting is definitely preferred :)

Thank you very much for catching this!

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:135: true);
On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> Why does document get throwing method stubs, but window doesn't?



I treated the document methods differently because the HTML5 specification says
document.write is evil and may throw exceptions in some cases, so I figured it's
OK to keep the current behavior of throwing exceptions.
http://www.w3.org/TR/html5/dom.html#dom-document-write

Most importantly, code that relies on feature detection to call these methods
(especially on document.write, the most popular one) will break in a standard
browser environment, so it's OK if it breaks in a Chrome App.

I'm not too hung up on the idea of throwing exceptions here. Some browsers just
ignore the calls, so I'd be happy with that solution as well. I care more about
"sane" methods not throwing in Chrome Apps.

What do you think?

https://codereview.chromium.org/120733003/diff/90001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/restrictions/main.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/restrictions/main.js:112:
window.dispatchEvent(event);
On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> Can you comment how this tests something?

Done.

Does this sound reasonable?

[pwnall-personal, 2014-01-08 23:30:31]

On 2014/01/08 23:20:45, pwnall wrote:
> Thank you very much for the feedback, Jeffery! Can you please take another
look?

I meant Jeffrey :( I'm really sorry for the typo!

So much for trying to claim that I'm detail-oriented, and I carefully analyzed
each one of the functions whose behavior is impacted by the change :/

[Jeffrey Yasskin, 2014-01-09 00:44:19]

lgtm with a couple comments, thanks!

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:52: function
disableMethods(object, objectName, methodNames, useThrowingStubs) {
On 2014/01/08 23:20:46, pwnall wrote:
> On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> > It seems like it would make sense for a *call* to a disabled method to
always
> > throw something, since if the method were absent (undefined), the call would
> > throw a TypeError. It's only getters that it makes sense to use a
> console.error
> > for, right?
> 
> Completely agreed, if getting the method from the object would return
> "undefined". Since we can't do that for non-configurable properties, I don't
> think it's OK to throw an exception on call.
> 
> I think the following code follows responsible feature detection practices,
and
> shouldn't break.
> 
> if (window.confirm) {
>     var response = window.confirm("Are you sure you want to delete this?");
>     ...
> }

Oh, right. Yes, your change makes sense. Maybe comment that, or use your example
in the test, so that a future maintainer doesn't "fix" it?

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:135: true);
On 2014/01/08 23:20:46, pwnall wrote:
> On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> > Why does document get throwing method stubs, but window doesn't?
> 
> 
> 
> I treated the document methods differently because the HTML5 specification
says
> document.write is evil and may throw exceptions in some cases, so I figured
it's
> OK to keep the current behavior of throwing exceptions.
> http://www.w3.org/TR/html5/dom.html#dom-document-write
> 
> Most importantly, code that relies on feature detection to call these methods
> (especially on document.write, the most popular one) will break in a standard
> browser environment, so it's OK if it breaks in a Chrome App.
> 
> I'm not too hung up on the idea of throwing exceptions here. Some browsers
just
> ignore the calls, so I'd be happy with that solution as well. I care more
about
> "sane" methods not throwing in Chrome Apps.
> 
> What do you think?

I don't have a strong opinion here at all (i.e. do what you want after thinking
about it), but it sounds like write() and writeln() are evil, so we shouldn't
mind throwing on calls to them, but maybe open(), clear(), and close() are less
evil, so we should be more careful for them?

[pwnall-personal, 2014-01-09 05:47:25]

Thank you very much for the thorough feedback, Jeffrey! I'll send this to the CQ
soon.

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:52: function
disableMethods(object, objectName, methodNames, useThrowingStubs) {
On 2014/01/09 00:44:19, Jeffrey Yasskin wrote:
> On 2014/01/08 23:20:46, pwnall wrote:
> > On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> > > It seems like it would make sense for a *call* to a disabled method to
> always
> > > throw something, since if the method were absent (undefined), the call
would
> > > throw a TypeError. It's only getters that it makes sense to use a
> > console.error
> > > for, right?
> > 
> > Completely agreed, if getting the method from the object would return
> > "undefined". Since we can't do that for non-configurable properties, I don't
> > think it's OK to throw an exception on call.
> > 
> > I think the following code follows responsible feature detection practices,
> and
> > shouldn't break.
> > 
> > if (window.confirm) {
> >     var response = window.confirm("Are you sure you want to delete this?");
> >     ...
> > }
> 
> Oh, right. Yes, your change makes sense. Maybe comment that, or use your
example
> in the test, so that a future maintainer doesn't "fix" it?

Done.
Thank you very much for the suggestion!

I added some comments to the method description.

There are some tests for the current behavior in one of the tests, but they
don't document the reasoning behind them.
https://codereview.chromium.org/120733003/patch/440001/450004

I think it makes more sense to have the reasoning in the source code, because I
expect people wanting to change the behavior to look at the source before they
look into updating tests.

https://codereview.chromium.org/120733003/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/platform_app.js:135: true);
On 2014/01/09 00:44:19, Jeffrey Yasskin wrote:
> On 2014/01/08 23:20:46, pwnall wrote:
> > On 2014/01/08 22:49:17, Jeffrey Yasskin wrote:
> > > Why does document get throwing method stubs, but window doesn't?
> > 
> > 
> > 
> > I treated the document methods differently because the HTML5 specification
> says
> > document.write is evil and may throw exceptions in some cases, so I figured
> it's
> > OK to keep the current behavior of throwing exceptions.
> > http://www.w3.org/TR/html5/dom.html#dom-document-write
> > 
> > Most importantly, code that relies on feature detection to call these
methods
> > (especially on document.write, the most popular one) will break in a
standard
> > browser environment, so it's OK if it breaks in a Chrome App.
> > 
> > I'm not too hung up on the idea of throwing exceptions here. Some browsers
> just
> > ignore the calls, so I'd be happy with that solution as well. I care more
> about
> > "sane" methods not throwing in Chrome Apps.
> > 
> > What do you think?
> 
> I don't have a strong opinion here at all (i.e. do what you want after
thinking
> about it), but it sounds like write() and writeln() are evil, so we shouldn't
> mind throwing on calls to them, but maybe open(), clear(), and close() are
less
> evil, so we should be more careful for them?

That makes perfect sense, thank you!

I updated the CL to do exactly that. Unfortunately, now we don't have a good way
to test that document.close() and document.clear() are stubbed, but I think the
tests cover the behavior that we actually care about.

In the future, if we want more precise testing, we can check that
document.close.toString() does not contain "[native method]", but that seems
hacky and not worth it for now.

[commit-bot: I haz the power, 2014-01-09 06:56:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/120733003/540001

[commit-bot: I haz the power, 2014-01-09 08:32:12]

Change committed as 243817