[chromium] Prevent JavaScript busy-loops in unload handlers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Abd-El-Malek]

This is causing poor user experience.

[John Abd-El-Malek]

BTW more background about this is available at http://code.google.com/p/chromium/issues/detail?id=7823

[John Abd-El-Malek]

Created attachment 39587 [details]
Proposed patch

[Sam Weinig]

It seems like the new test will fail with JSC.

[John Abd-El-Malek]

(In reply to comment #3)
> It seems like the new test will fail with JSC.

yes of course, thanks for noting.  I had modified the skipped files of all the other ports but failed to recreate those changes after moving this change to another machine.  Will upload a newer patch now.

[John Abd-El-Malek]

Created attachment 39590 [details]
Update Skipped files for other platforms

[Adam Barth]

Comment on attachment 39590 [details]
Update Skipped files for other platforms

This patch has a number of strange side effects.  Most notably, the result of Date.prototype.getTime.toString() will change from its normal value to something wonky at odd times.

[Sam Weinig]

Is it really a good idea for us to diverge here?

[Adam Barth]

Another wonky thing is that you leak this powerful function that calls v8::V8::TerminateExecution() to the page.  I'm not entirely sure what evil things the page might do with that function, but it seems like a bad idea.

[John Abd-El-Malek]

(In reply to comment #8)
> Another wonky thing is that you leak this powerful function that calls
> v8::V8::TerminateExecution() to the page.  I'm not entirely sure what evil
> things the page might do with that function, but it seems like a bad idea.

Do you mean indirectly leak by calling getTime 1K times in an unload handler?  The page can't do anything that will call this function otherwise, since the only pointer to it is in the closure.

Re getTime.toString showing different results.  I think this is pretty minor, especially compared to the problems that this solves (check out the chromium bug link for comments on how big the problem is, and other drastic changes that people wanted to do to prevent it).  It won't be possible to do this with no side effects unless we have V8 changes, which the V8 team doesn't think is necessary.  They are happy with this approach (Mads reviewed it).

This is a stop-gap measure until we implement <a ping> and evangelize it.

[John Abd-El-Malek]

(In reply to comment #7)
> Is it really a good idea for us to diverge here?

I had emailed you and Maciej a few weeks ago about this, to see if you're interested in doing something that works for both Safari and Chrome, but I never heard back so I did something just for Chrome.  If you'd like something like this for JSC based browsers, I'd be happy to look into it.  Otherwise I think this doesn't preclude Chrome from solving this problem for now.  This is a big issue, i.e. some pages have upwards of 2 seconds of sleep in their unload handlers.

[Adam Barth]

Comment on attachment 39590 [details]
Update Skipped files for other platforms

I really don't think this is the right approach.  This doesn't actually solve the problem.  Sites that want to sleep in their unload handlers will just resort to a Yet More Ridiculous Hack and save the original getTime function.

The proper way to solve this problem is to give V8 a time bound for running the unload handler and have V8 terminate the script internally.  Hacking around with the global JS environment is prone to disaster.

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> The proper way to solve this problem is to give V8 a time bound for running the
> unload handler and have V8 terminate the script internally.  Hacking around
> with the global JS environment is prone to disaster.

Time based is not good enough given the resolution we are talking about here.  We want to be able to terminate after ~100ms worth of execution, but at that time scale, we'll get a lot of false positives due to the thread being interrupted by other processing on the system.  So, we need to count something.  Maybe counting getTime calls is not the best solution, but we need to count something other than elapsed wall clock time.

[Adam Barth]

(In reply to comment #12)
> So, we need to count something.

Does V8 have a notion of number of operations?  Maybe you should just bound the total number of operations.  I think you should assume that the site is going to try to work around this limitation because sites are already using ridiculous hacks as is.

[Dimitri Glazkov (Google)]

My sincere hope here was to avoid retaliatory cycle of workarounds from Web developers, because the change doesn't break their behavior: the network requests they are waiting for to complete should have ample time to do so. So maybe they won't even notice we're doing this?

[Adam Barth]

Wow, that crbug thread is a monster.  I presume you've investigated other options like just hiding the tab while the unload handler runs (or after the unload handler has been running for too long).

[John Abd-El-Malek]

(In reply to comment #15)
> Wow, that crbug thread is a monster.  I presume you've investigated other
> options like just hiding the tab while the unload handler runs (or after the
> unload handler has been running for too long).

Yeah, a lot of the context is in that bug, and also in hallway/email/rietveld discussions with Darin/Mike/Mads/Dimitri and others.

Hiding the tab has its own issues (i.e. what if an alert/prompt/showModalDialog is shown?), and also doesn't fix the navigation use case.

As I mentioned in the crbug thread, this isn't meant as a fool-proof workaround, since that's a losing battle (i.e. sites can just compute primes, or register a thousand handlers to defeat any limit on an individual running time/cycles of a handler).  This bug is a testament to what web developers will do to workaround any browser limitations.  The result is a much worse user experience.

This is only a stop gap until we have <a ping>, which I'll be looking at soon.

[Adam Barth]

John is going to send me all the info, and I'll post a summary here.

[Adam Barth]

Ok.  I've read all the relevant info.  As in most things, agree with Darin.  Some background:

1) A number of web pages are super slow to close because they are lame and chew up a lot of time in their unload handlers in order to get some network pings to the server for analytics.

2) These scripts are optimized to evade common browser defenses for slow script.  For example, one ad network adds a bunch of 200ms unload event handlers to avoid the slow script dialog.

3) John is excited about tricking these scripts by manipulating the clock, but no one else seems excited by that plan because of the following concerns:
  a) Not all the unload scripts use Date to sleep.
  b) These checks can be worked around, creating an arms race that we cannot win.

4) To my mind, a better approach is that proposed by Peter in http://code.google.com/p/chromium/issues/detail?id=7823#c20 (and supported by Linus in http://code.google.com/p/chromium/issues/detail?id=7823#c49) is to hide tabs once the user clicks the close button.  John points out this has a number of strange corner cases involving zombie tabs reappearing from their hidden state.

5) The design proposed by Darin in http://code.google.com/p/chromium/issues/detail?id=7823#c58 seems workable.  To summarize here:
  a) Run beforeunload as normal.
  b) Before invoking unload, hide the tab.
  c) If the page attempts to alert / confirm / rise from the dead, ignore it.

I think the patch attached to this bug is a giant hack that doesn't solve the problem.

[Darin Fisher (:fishd, Google)]

Adam, the hide the tab solution doesn't help for frame navigations.

[Peter Kasting]

(In reply to comment #18)
> 4) To my mind, a better approach is that proposed by Peter in
> http://code.google.com/p/chromium/issues/detail?id=7823#c20 (and supported by
> Linus in http://code.google.com/p/chromium/issues/detail?id=7823#c49) is to
> hide tabs once the user clicks the close button.  John points out this has a
> number of strange corner cases involving zombie tabs reappearing from their
> hidden state.

Note that this does not solve the problem of navigating within the same tab (which Darin has just noted).  For that, my proposal is basically that we look to see if the old page was the only entity that would have been able to script the new page (the common case), and if it was, we launch the new link in a different process, which we let use this tab to render.  Meanwhile we run the unload handler for the old tab.  The same sorts of issues as in the tab-closing case apply: resurrecting hidden tabs and similar.

I also proposed we first give the renderer 100 ms to bring up UI (during which we don't hide/swap out the tab) and then don't let it do so afterward (which prevents the "zombie UI" issues), but Darin/John pointed out that if part of the renderer was swapped out, it would be impossible to determine that the renderer had had a fair opportunity to swap back in and work long enough to avoid dataloss.  I'm not sure I have a way around that.

I hate the sleep()-killing idea but it might work (?).  However, doing it assumes that we get <a ping> implemented in WebKit and Gecko, released to the public, and evangelized to developers as the alternative here.  That needs to happen anyway, of course, but I am leery of doing this change before then.

I am also more worried than both Darin and John about repercussions from doing this (sites blacklisting Chrome/WebKit, underreporting of UAs so that sites don't bother to make themselves work with Chrome/WebKit).

Perhaps another idea, if we think that waiting for resource loads to complete is what all sleep()-ing scripts will do, is to transparently convert loads in unload handlers into <a ping>-type actions, and then go ahead and kill the handlers.

[Adam Barth]

Regardless of whether you think this is the right approach (which I don't), WebCore is the wrong layer to make this change: WebCore ought not even know that the ECMAScript Date API exists.

[John Abd-El-Malek]

I think it's unfortunate that, as a result of Chrome's code residing in two different repositories, we need to have the same discussion twice in two different bug systems, and once it's approved in one code review, it has to go through all this again.  The bug was open for half a year in Chrome's bug tracker, and the code review was sent in the open there.  The bugs and code reviews also don't capture all the hallway conversations that occurred.

The same questions and points are made over and over.  At this point, I'm really tired of explaining things multiple times.  If people want to discuss this, they can find me in-person, or they can invest the effort in reading all the previous discussions fully.

[Peter Kasting]

(In reply to comment #22)
> I think it's unfortunate that, as a result of Chrome's code residing in two
> different repositories, we need to have the same discussion twice in two
> different bug systems, and once it's approved in one code review, it has to go
> through all this again.

I've seen no code review on that Chromium bug, and I've never agreed that it's the right approach there.  And at least most of the people involved here (you, me, Darin) have been involved in that discussion since the beginning.  So I'm not sure what grounds you have to effectively accuse us of being ignorant of what's happening.

[Peter Kasting]

Darin (Fisher), John and I had a long chat this morning in which we discussed the ramifications of this and possible alternatives, as well as future plans.  In short, I now support this change, although I admit it is a hack.  Gory detail below if you care.

First, regarding the question of whether this change will solve the problem.  According to John, comment 18 point 3a is inaccurate: basically all unload handlers use this same technique to busy-loop.  Therefore, initially, this change will make unload handlers run much faster.  As Adam notes, just doing this alone could potentially lead to an arms race.  There are two reasons to believe this is unlikely:

1) We propose to guarantee that by the time we ship this in a Stable channel release, we will also have <a ping> available in the same release.  This gives authors who care about our behavior an easier and more-guaranteed-than-the-old-way mechanism.  Anyone who cares enough about Chromium's behavior to rewrite their code will have significant motivation to use this.

2) In the meantime, Chromium's market share (and, to be honest, WebKit's in general) is too low for most authors to care.

Second, regarding future plans.  If we land this, we propose to also land some logging/histograms of unload handler wall clock time and/or the number of times this mechanism fires, so that we can measure its efficacy and the uptake of <a ping>.  This can help us decide whether an arms race is actually occurring (and if so what to do), or whether the hack has become sufficiently unnecessary that we can just rip it out.

Third, regarding divergence.  As John mentioned, it's perfectly reasonable to make this change for JSC too, and in fact I would prefer it, as it would be to users' benefit and make the behavior of "WebKit" more consistent from an author perspective.  However, note that the recent change to make pages with unload handlers eligible for the bfcache means that unload handlers will rarely fire where that is supported, meaning that authors may copy this code into the pagehide handler, meaning that the hack might need to be expanded to also cover pagehide for platforms with a bfcache.

Fourth, regarding alternate solutions.  I have been convinced by Darin that all alternative solutions currently conceived are inferior in some way or other.  Here are a scattering of examples:

* Hiding the page/tab while the handler is running suffers from numerous problems, such as what to do if the handler fires an alert(), how to do this if navigation is within the same domain or multiple windows can script this one, and the jankiness caused by the renderer spinning the CPU and/or doing resource loads in the background while the user is trying to load a new page.

* Trying to halt script using a different, less-workaroundable method such as wall clock time, or instruction or function call count, suffer from implementation problems, such as wall clock time being a poor bound if the renderer ever has to swap from disk or gets suspended by the OS, or raw instruction count access imposing significant constraints on the JS engine design and API.  The V8 folks have indicated that this level of granularity will cause them problems now and in the future, and I imagine similar things might be true for JSC.

* In general, approaches which transparently allow authors to continue using a mechanism like the current one provide authors no motivation to switch to <a ping> (which would benefit users even if such approaches were implemented), and effectively force all browser vendors to either copy this kind of hack or continue to let their users suffer.  Providing both a carrot _and_ some kind of stick should increase the likelihood of ending up in a better world, since authors must hate the current code as it is fiddly and still doesn't actually guarantee them that their tracking pings will complete.

* Not that this would be strong evidence by itself, but a number of different Chromium team members familiar with different aspects of this (web authoring, WebCore, V8, UI, etc.) have discussed this solution and its alternatives and generally agreed that this is the best route.

Regarding the low-level details of how this patch is implemented, I am not knowledgeable enough to comment, so I will leave that to Darin or John.

[Adam Barth]

I've also discussed this with Darin and John (although not Peter directly).

1) I think we should give web developers a bigger carrot to avoid an arms race here.  In particular, we should consider completing network requests started from unload handlers, removing the incentive to use a busy-loop.

2) I continue to think that WebCore is the wrong layer at which to make a change that manipulates the Date API.  The reasons to prefer making the change here appear more political than technical.

[John Abd-El-Malek]

(In reply to comment #25)
> I've also discussed this with Darin and John (although not Peter directly).
> 
> 1) I think we should give web developers a bigger carrot to avoid an arms race
> here.  In particular, we should consider completing network requests started
> from unload handlers, removing the incentive to use a busy-loop.

There are a variety of serious problems with this.  Peter just touched on some of them.  This been discussed to death already.

> 
> 2) I continue to think that WebCore is the wrong layer at which to make a
> change that manipulates the Date API.  The reasons to prefer making the change
> here appear more political than technical.

The (technical) reasons why this is the case have been made a number of times.  V8 team does not want to add hooks as it's too one-off, and it limits their flexibility.  We will need hooks anyways in the bindings layer to know when we're entering/exiting an unload handler.

[Adam Barth]

(In reply to comment #26)
> > 1) I think we should give web developers a bigger carrot to avoid an arms race
> > here.  In particular, we should consider completing network requests started
> > from unload handlers, removing the incentive to use a busy-loop.
> 
> There are a variety of serious problems with this.  Peter just touched on some
> of them.  This been discussed to death already.

Are you referring to the passage where Peter says doing this provides authors no incentive to switch to <a ping>?  Darin also noted on IRC that this might slow down loading a new page from the same server.

I think the main disagreement here is over how we think authors will respond to various incentives.  I don't think any of us know for sure.  As you've pointed out, they might even just ignore us given our market share.

> > 2) I continue to think that WebCore is the wrong layer at which to make a
> > change that manipulates the Date API.  The reasons to prefer making the change
> > here appear more political than technical.
> 
> The (technical) reasons why this is the case have been made a number of times. 
> V8 team does not want to add hooks as it's too one-off, and it limits their
> flexibility.  We will need hooks anyways in the bindings layer to know when
> we're entering/exiting an unload handler.

I could say the same thing here.  Why not implement this in the browser process by sending an evalJavaScript IPC message?  The only reason this patch is technically possible is because the Date API happens to be stored in a global variable.  Do you really think this patch isn't breaking the abstraction barrier?  (That's even ignoring all the web-visible side-effect of monkeying around with the global namespace.)

[Peter Kasting]

(In reply to comment #27)
> (In reply to comment #26)
> > > 1) I think we should give web developers a bigger carrot to avoid an arms race
> > > here.  In particular, we should consider completing network requests started
> > > from unload handlers, removing the incentive to use a busy-loop.
> > 
> > There are a variety of serious problems with this.  Peter just touched on some
> > of them.  This been discussed to death already.
> 
> Are you referring to the passage where Peter says doing this provides authors
> no incentive to switch to <a ping>?  Darin also noted on IRC that this might
> slow down loading a new page from the same server.

Yes, I tried to mention both those issues in my comment, as well as the issues that this can also slow down _other_ pages being loaded as well as all other actions in the browser, and is also deleterious for other browser vendors and does not result in the fastest possible tab closure.

Explicitly note that in IE, this is already the case (for image loads), and yet authors busy loop in their unload handlers without regard to what browser is in use.  Making the busy loop unnecessary is not nearly as good as providing a far better solution at the same time that you make the busy loop not function.

In summary, this isn't a workable solution.  I have fought for it and thought around it for quite some time and I can't support it.

> I think the main disagreement here is over how we think authors will respond to
> various incentives.  I don't think any of us know for sure.  As you've pointed
> out, they might even just ignore us given our market share.

There are three possible outcomes:
* Authors completely ignore this, in which case we have made users' lives better.
* Authors move from the current code to <a ping>, in which case we have made users of _all_ browsers' lives better, and made authors' lives better as well (better guarantees on load completion).
* Authors ignore <a ping> and respond to this by finding some other way to busy loop, in which case things are generally unchanged.

None of these things actively makes things terribly _worse_, and in the worst case (case 3), we can choose how to respond, for example by counting more types of instructions.

As I tried to note above, the current code doesn't work well for authors, so their incentive to move to <a ping> once it's supported is greater than just "we're going to make your current code not work".

[Adam Barth]

> In summary, this [completing network requests] isn't a workable solution.
> I have fought for it and thought around it for quite some time and I can't
> support it.

In any case, it doesn't block this patch because its something we can decide to do (or not do) in another patch.

[Peter Kasting]

(In reply to comment #29)
> > In summary, this [completing network requests] isn't a workable solution.
> > I have fought for it and thought around it for quite some time and I can't
> > support it.
> 
> In any case, it doesn't block this patch because its something we can decide to
> do (or not do) in another patch.

Yes, implementing this as a first pass would not stop us from modifying (or removing) this later if something else is found to be more effective.  More interesting to me is the question of whether there is any possible harm to authors due to your comment 6 or comment 8 (although it sounds like perhaps comment 8 is a nonissue since authors can't actually get at this?).  Ideally the patch would be one which doesn't have any harmful side effects.  That is my greatest concern.

[Adam Barth]

(In reply to comment #30)
> More interesting to me is the question of whether there is any possible harm to
> authors due to your comment 6 or comment 8 (although it sounds like perhaps
> comment 8 is a nonissue since authors can't actually get at this?).

It depends what you mean by harm to authors.  I don't think anyone is going to suffer any monetary loss because the toString of a function magically changes at some odd time.  On the outer hand, the web platform would be even more insane if we did that all the time.

W.r.t. other technical issues with the patch, the use of the |this| keyword is quite dangerous to secure JavaScript subsets like ADsafe or Caja because |this| can point to the global object.  They take great care to dance around the uses of |this| in the JS implementation in Firefox.  Adding |this| to this super-obscure corner case is like hiding a land mine in your corn field: eventually it will blow your legs off.

As for leaking the terminateScript capability, I don't have a good sense of what evil things a page could do with that.  I'd rather not have to worry about those issue.

> Ideally
> the patch would be one which doesn't have any harmful side effects.  That is my
> greatest concern.

The reason we have to worry about this stuff is because the patch is written at the wrong layer of abstraction.  It's the same reason we could steal all the user's passwords from 6 of 6 password managers built out of (essentially) the evalJavaScript IPC message:  http://www.adambarth.com/papers/2009/adida-barth-jackson.pdf

To do this right, you need to be inside the engine, not splicing loaded guns into a potentially malicious environment.

[Adam Barth]

Sorry, I should provide more context.  The issues in Comment #30 are all small details.  They'd be much bigger problems if we tried to build our whole system this way.

We're likely to get away with this case because the exposure is so obscure.  For example, it's unlikely someone would run code from an page module during unload.  (They could, of course, but they probably wont.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 39590 [details]
Update Skipped files for other platforms

I support proceeding with this change.  I think we reached consensus on
webkit-dev to also implement detached Image loads as the big carrot to
accompany this big stick.  I'm fine with making these changes in separate
patches, so long as we get to the carrot soon :-)

Here's some style nits:


> Index: WebCore/bindings/v8/DateExtension.cpp
...
> +DateExtension* DateExtension::s_extension;
> +
> +static const char* s_dateExtensionName = "v8/DateExtension";
> +static const char* s_dateExtensionScript =

nit: WebKit style is to just name static variables the same way you name
local variables, so drop the "s_"


> +v8::Handle<v8::FunctionTemplate> DateExtension::GetNativeFunction(v8::Handle<v8::String> name)
> +{
> +    if (name->Equals(v8::String::New("GiveEnableSleepDetectionFunction"))) {
> +        return v8::FunctionTemplate::New(GiveEnableSleepDetectionFunction);
> +    } else if (name->Equals(v8::String::New("OnSleepDetected"))) {
> +        return v8::FunctionTemplate::New(OnSleepDetected);
> +    }

nit: WebKit style is to not use {}'s around single line statements


> +void DateExtension::weakCallback(v8::Persistent<v8::Value> object, void* param)
> +{
> +    DateExtension* extension = get();
> +    for (FunctionPointerList::iterator i = extension->callEnableSleepDetectionFunctionPointers.begin();
> +         i != extension->callEnableSleepDetectionFunctionPointers.end(); ++i) {
> +        if (*i == object) {
> +            i->Dispose();
> +            extension->callEnableSleepDetectionFunctionPointers.erase(i);
> +            return;
> +        }
> +    }
> +    ASSERT(false);

nit: ASSERT_NOT_REACHED()


> Index: WebCore/bindings/v8/DateExtension.h
...
> +#ifndef DateExtension_h
> +#define DateExtension_h
> +
> +#include <v8.h>
> +
> +#include <list>

nit: Please avoid using STL containers or anything from STL that
allocates memory.  This will matter to Android, which does not have
a complete STL implementation.

maybe you could use WTF's Vector or Deque containers instead?

[Peter Kasting]

(In reply to comment #33)
> > +v8::Handle<v8::FunctionTemplate> DateExtension::GetNativeFunction(v8::Handle<v8::String> name)
> > +{
> > +    if (name->Equals(v8::String::New("GiveEnableSleepDetectionFunction"))) {
> > +        return v8::FunctionTemplate::New(GiveEnableSleepDetectionFunction);
> > +    } else if (name->Equals(v8::String::New("OnSleepDetected"))) {
> > +        return v8::FunctionTemplate::New(OnSleepDetected);
> > +    }
> 
> nit: WebKit style is to not use {}'s around single line statements

Further nit: Not mentioned in WebKit's style guide, but sometimes advocated by them, and also advocated by Google (I think), is to avoid "else" after "return".

[John Abd-El-Malek]

Created attachment 39873 [details]
Updated patch

Addressed all comments.  I had used std::list because I saw it in V8Proxy, but it's not even needed there at all since there's no removal.  I can fix it in another change.

[John Abd-El-Malek]

Created attachment 39876 [details]
Might as well update V8Proxy to not use STL now since I'm touching that code

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/48612