On Mon, Sep 21, 2009 at 5:32 PM, Drew Wilson <[email protected]> wrote:
> Ooops, sorry. Got bit by reply vs reply-all. See my notes below regarding > implementation details. > -atw > > ---------- Forwarded message ---------- > From: Drew Wilson <[email protected]> > 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 <[email protected]> > > > > > On Mon, Sep 21, 2009 at 2:52 PM, Mike Belshe <[email protected]> wrote: > >> On Mon, Sep 21, 2009 at 2:31 PM, Jeremy Orlow <[email protected]>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. > I really hate adding yet more custom code. There are already sooo many steps to adding a feature bindings wise. And it'll be very easy for people to screw it up. I guess tests can help, though... Also, I'm not sure I agree that side-effects in enumeration is OK if we're shipping this stuff in stable. This will bite us at some point or another. >> >>> 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: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
