On Mon, Sep 21, 2009 at 5:41 PM, Jeremy Orlow <jor...@chromium.org> wrote:
> On Mon, Sep 21, 2009 at 5:32 PM, Drew Wilson <atwil...@chromium.org>wrote: > >> 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. >> > > 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. > If we were leaving things in "experimental" state for long periods of time, then sure. But given that typically things hang out in experimental for no more than a quarter or two (typically for a web feature that isn't widely deployed on other browsers yet either) and people generally don't use enumeration for capabilities testing, I suspect we'll never run into any problems with it. That said, I certainly would not discourage people from submitting patches to address this - it just seems like a bunch of work for something that's unlikely to cause compatibility problems in practice, so I wasn't planning to do anything in that area. > > >>> >>>> 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 -~----------~----~----~----~------~----~------~--~---
