Ooops, sorry. Got bit by reply vs reply-all. See my notes below regarding implementation details. -atw
---------- Forwarded message ---------- From: Drew Wilson <atwil...@google.com> Date: Mon, Sep 21, 2009 at 5:03 PM Subject: Re: [chromium-dev] Shipping features behind a run-time flag can sometimes still be dangerous To: Mike Belshe <mbel...@google.com> On Mon, Sep 21, 2009 at 2:52 PM, Mike Belshe <mbel...@google.com> wrote: > On Mon, Sep 21, 2009 at 2:31 PM, Jeremy Orlow <jor...@chromium.org> wrote: > >> I think we need to re-consider our practice of shipping beta/stable >> browsers with experimental features hidden behind flags--at least when they >> have any side-effects in JavaScript. An example of where this has bitten us >> is http://code.google.com/p/chromium/issues/detail?id=22181 >> >> Although part of the problem is the way they coded things (since both >> SessionStorage and LocalStorage use the Storage interface, >> its existence doesn't imply SessionStorage is necessarily available), this >> bug has pointed out a couple problems. 1) constructors are visible to >> javascript even when the feature is totally disabled. >> > > If it's behind a flag, it shouldn't have been exposed, right? On the > surface, it sounds like this code was only partially hidden behind the flag? > > I think it would be a good idea to have a unit test which enumerates all > symbols that we're exposing into JS. This should be a controlled list. > > If we had this unit test, would it have caught this exposure? > I believe this test exists (fast/dom/Window/window-properties.html). I suspect the problem is that people aren't zealously reviewing its contents to check for leaks. Anyhow, it's trivial to change the code generator for a given attribute to map null to undefined, which is what I was planning to do for SharedWorkers. It's somewhat trickier to generate code to enable/disable constructors since there's probably not a common way to tell if a given constructor should be enabled or not (probably simpler just to make the constructor-getter v8-custom, and have custom code to return undefined). Likewise, looks like there's a DontEnum flag we can set on the various prototype table values to hide them from "for...in" - the trick is how to initialize these tables appropriately (and in a thread-safe way), given that they are statically defined. Disabling enumeration seems like quite a bit of work for not much benefit so I'd push back on doing anything there. I'd say that if we want to hide constructors, we should do it via custom bindings rather than trying to build some general solution since you'd likely need a custom "see if feature X is enabled" code block anyway. > > Mike > > > > > > >> 2) When an object (like the Storage interface) wraps a NULL it shows up >> as null in JavaScript. Since returning NULL/0 is the standard thing to do >> when the feature is disabled, this means that the functions return null when >> disabled at run time and undefined when disabled at compile time. 3) Even >> if we fixed the undefined problem, |'localStorage' in window| would still >> return true. >> >> We've been discussing these issues in a WebKit-dev thread ( >> https://lists.webkit.org/pipermail/webkit-dev/2009-September/thread.html#9860) >> and although (2) is probably something we can solve without too much effort >> (Drew is going to look into it), (1) and (3) probably aren't worth changing >> if the runtime flag is just temporary. >> >> *As such, I feel fairly strongly that we should start disabling features >> behing a run-time flag with compile-time flags in future beta/stable builds >> if they have any side-effects in JavaScript.* I'm working with Anthony >> LaForge to fix this for LocalStorage/SessionStorage right now. I'm not sure >> if it's worth doing preemptively for other features, but it might be. I >> definitely think we should do it the next time we cut a beta build. >> Especially if there's a chance the beta will be an ancestor of a stable >> channel release. >> >> J >> >> >> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
