Bug 81506 – Asserts in XMLHttpRequestProgressEventThrottle

• Component: XML
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

XMLHttpRequests when active DOM objects have been suspended will often cause asserts in  XMLHttpRequestProgressEventThrottle.

This is most easily triggered using Qt WebKit2 which suspends active DOM objects on panning, and loading a page which uses a XMLHttpRequests. For instance index.hu. The bug is not specific to this type of suspend theory, and could in theory also be triggered on pagecache or JavaScript alert.

[Allan Sandfeld Jensen]

Created attachment 132568 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 132568 [details]
Patch

r- due to lack of regression test.

Please also explain why you're receiving data from network in suspended state.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 132568 [details])
> r- due to lack of regression test.
> 
> Please also explain why you're receiving data from network in suspended state.

Okay, I have made a test-case based on pagehide event.

Data does not need to arrive from anywhere for readystate to change. This is the incorrect assumption the current asserts make.

Something as simple as calling xmlhttprequest::open() on any URL will cause a readystate event-change (to OPENED). This is not a timed event, and has nothing to do with incoming network-traffic.

[Allan Sandfeld Jensen]

Created attachment 132619 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 132619 [details]
Patch

Not suitable for review. Upload scripts collected irrelevant changes.

[Allan Sandfeld Jensen]

Created attachment 132791 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132791 [details]
Patch

Attachment 132791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12002218

New failing tests:
fast/events/pagehide-xhr-open.html

[Allan Sandfeld Jensen]

Created attachment 132811 [details]
Patch

[Alexey Proskuryakov]

I'd like to take a deep look at this, but I won't be able to in the next few days. Julien, what do you think about this change?

[Julien Chaffraix]

(In reply to comment #9)
> I'd like to take a deep look at this, but I won't be able to in the next few days. Julien, what do you think about this change?

Sure. I had a look but I would like more explanation before being able to review that:

> Something as simple as calling xmlhttprequest::open() on any URL will cause a readystate event-change (to OPENED). This is not a timed event, and has nothing to do with incoming network-traffic.

Shouldn't JS be also suspended in this case? I think I don't get why we still get some calls to XMLHttpRequestProgressThrottle after suspend() was called and an explanation along those lines would help.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> (In reply to comment #9)
> > I'd like to take a deep look at this, but I won't be able to in the next few days. Julien, what do you think about this change?
> 
> Sure. I had a look but I would like more explanation before being able to review that:
> 
> > Something as simple as calling xmlhttprequest::open() on any URL will cause a readystate event-change (to OPENED). This is not a timed event, and has nothing to do with incoming network-traffic.
> 
> Shouldn't JS be also suspended in this case? I think I don't get why we still get some calls to XMLHttpRequestProgressThrottle after suspend() was called and an explanation along those lines would help.

On the API layer, suspending JS and suspending ActiveDOMObjects are two different calls, so should not depend on each other. In practice though, there are as far as I know only two places only ActiveDOMObjects are suspended: While evaluating onpagehide events, and in QtWebKit2 and iOS while handling animated touch gestures.

[Julien Chaffraix]

Comment on attachment 132811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132811&action=review

Thanks Allan. In light of these explanations, I think the patch can be made better.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:55
> +    if (suspended()) {
> +        m_pausedEvent = XMLHttpRequestProgressEvent::create(eventNames().progressEvent, lengthComputable, loaded, total);
> +        return;
> +    }

You mentioned that this is not called because of network activity after suspend() so I wonder if / why this is needed. progress events should be the result of network activities only.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:-77
>  void XMLHttpRequestProgressEventThrottle::dispatchEvent(PassRefPtr<Event> event, ProgressEventAction progressEventAction)
>  {
> -    ASSERT(!suspended());
> -    // We should not have any pending events from a previous resume.
> -    ASSERT(!m_pausedEvent);
> -

I think this is a design error I made when creating the XMLHttpRequestProgressEventThrottle: XMLHttpRequest dispatches all its events through here and this is bad. Especially since I wouldn't expect this function to be called on a suspended throttle.

As you said, JS can still make us dispatch events so the right fix would keep those ASSERT around to still catch some badness, make this function private and have the XMLHttpRequest object handle the event dispatching directly (calling flushProgressEvent as needed).

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:98
> +    if (m_pausedEvent) {
> +        m_target->dispatchEvent(m_pausedEvent);
> +        m_pausedEvent = 0;
> +        return;
> +    }

This really shouldn't be needed: if we are suspended(), the logic (including the one you added) should save the paused event and dispatch it during resume(). If we have a paused event, it means there is a race condition somewhere else.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 132811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132811&action=review
> 
> Thanks Allan. In light of these explanations, I think the patch can be made better.
> 
> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:55
> > +    if (suspended()) {
> > +        m_pausedEvent = XMLHttpRequestProgressEvent::create(eventNames().progressEvent, lengthComputable, loaded, total);
> > +        return;
> > +    }
> 
> You mentioned that this is not called because of network activity after suspend() so I wonder if / why this is needed. progress events should be the result of network activities only.
> 
Data does appear to arrive during suspend(), but it is not necessary for the asserts to trigger. One example is synchronous loads, and the other is when xmlhttprequest is forced to suspend even though it has an asynchronous load. Note that XMLHttpRequest::suspend makes no attempt to stop the threaded loader, and the ExecutionContext is allowed to ignore canSuspend(). Storing the latest progress-event is the best that can be done to avoid emitting timed events in these cases.

I was thinking of three ways to solve the problem:
1. Implement a pause/defer/suspend api on the threadedloader.
2. Pool all the readystatechanges trigger by new data, and only emit them later.
3. Try to keep the events as consistent as possible, if data does arrive during suspend.

Considering the case of synchronous data, I am not entirely sure it would be correct to assume data should never happen. So I went with 3. Also if the behaviour is wrong; it is a larger and different bug that just allowing readystate changes while suspended.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:-77
> >  void XMLHttpRequestProgressEventThrottle::dispatchEvent(PassRefPtr<Event> event, ProgressEventAction progressEventAction)
> >  {
> > -    ASSERT(!suspended());
> > -    // We should not have any pending events from a previous resume.
> > -    ASSERT(!m_pausedEvent);
> > -
> 
> I think this is a design error I made when creating the XMLHttpRequestProgressEventThrottle: XMLHttpRequest dispatches all its events through here and this is bad. Especially since I wouldn't expect this function to be called on a suspended throttle.
> 
> As you said, JS can still make us dispatch events so the right fix would keep those ASSERT around to still catch some badness, make this function private and have the XMLHttpRequest object handle the event dispatching directly (calling flushProgressEvent as needed).

Makes sense. I did not want to do so without your input though. Btw if XMLHttpRequestProgressEventThrottle ends up only emitting progress events, doesn't the generic dispatchEvent() become redundant, so the last code could be moved to dispatchProgressEvent?

> 
> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:98
> > +    if (m_pausedEvent) {
> > +        m_target->dispatchEvent(m_pausedEvent);
> > +        m_pausedEvent = 0;
> > +        return;
> > +    }
> 
> This really shouldn't be needed: if we are suspended(), the logic (including the one you added) should save the paused event and dispatch it during resume(). If we have a paused event, it means there is a race condition somewhere else.

I am not sure if it is needed either, but the existing code would emit the last progress if flushed (for instance a cancel ?), so this simply follows the same logic.

[Julien Chaffraix]

> > I think this is a design error I made when creating the XMLHttpRequestProgressEventThrottle: XMLHttpRequest dispatches all its events through here and this is bad. Especially since I wouldn't expect this function to be called on a suspended throttle.
> > 
> > As you said, JS can still make us dispatch events so the right fix would keep those ASSERT around to still catch some badness, make this function private and have the XMLHttpRequest object handle the event dispatching directly (calling flushProgressEvent as needed).
> 
> Makes sense. I did not want to do so without your input though. 

Btw, this is bug 46743 (which has a rotten patch for that). It sounds like it could be fixed first as a stepping stone to getting rid of the ASSERTs.

> Btw if XMLHttpRequestProgressEventThrottle ends up only emitting progress events, doesn't the generic dispatchEvent() become redundant, so the last code could be moved to dispatchProgressEvent?

Sounds good to me, especially since there is no need to flush pending progress events anymore.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> > > I think this is a design error I made when creating the XMLHttpRequestProgressEventThrottle: XMLHttpRequest dispatches all its events through here and this is bad. Especially since I wouldn't expect this function to be called on a suspended throttle.
> > > 
> > > As you said, JS can still make us dispatch events so the right fix would keep those ASSERT around to still catch some badness, make this function private and have the XMLHttpRequest object handle the event dispatching directly (calling flushProgressEvent as needed).
> > 
> > Makes sense. I did not want to do so without your input though. 
> 
> Btw, this is bug 46743 (which has a rotten patch for that). It sounds like it could be fixed first as a stepping stone to getting rid of the ASSERTs.
> 

Yes, what they that patch tries to prepare for is second solution I listed.

[Julien Chaffraix]

> I was thinking of three ways to solve the problem:
> 1. Implement a pause/defer/suspend api on the threadedloader.
> 2. Pool all the readystatechanges trigger by new data, and only emit them later.
> 3. Try to keep the events as consistent as possible, if data does arrive during suspend.

I think 2 is the way to go here as we can't make any assumptions on what will happen during suspend. We could implement 1. but it wouldn't get rid of the event dispatched by JavaScript which lead us in an inconsistent state from the web-app perspective (or we would have to implement 2. anyway). Please make sure that this design choice is mentioned in your patch.

[Allan Sandfeld Jensen]

Created attachment 133495 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 133814 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 133814 [details]
Patch

Renamed pausedEvents to deferredEvents and renamed m_suspened to m_deferEvents, since it no longer indicates suspend directly.

[Julien Chaffraix]

Comment on attachment 133814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133814&action=review

> Source/WebCore/ChangeLog:10
> +        Queues events attempted dispatched while the object is suspend. Coalescing progress and
> +        readystatechange events. Events are dispatched soon after the object has been resumed,
> +        but not immediately to avoid hitting a recursion issue in ScriptExecutionContext.

That's why I prefer detailed ChangeLog, this paragraph could be spread below to underline the important points. Here you would keep a summary of the change and why it's needed: JS can be enabled while being suspended and the loaders don't suspend! (I *really* would like this information included as it is paramount to understanding the new design)

> Source/WebCore/xml/XMLHttpRequest.cpp:1152
> +            if (!m_progressEventThrottle.suspended())

Why do we need that? That sounds like it's counter productive and would be better pushed down to the throttle.

As a side note, I just saw that suspended() is public and I think it really shouldn't.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:99
> +        m_deferredEvents.append(event);

The Vector can grow unbounded here. We should have a size limit above which we just drop the events.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:116
> +        m_deferredEvents.append(m_deferredProgressEvent);

Ditto.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:218
> +    startOneShot(0);

I really think this should be its own Timer with a dedicated function instead of re-using the existing logic, which feels clunky.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:77
> +    bool m_deferEvents;

I am not super enthusiastic about renaming m_deferEvents to m_suspended (I could be convinced the contrary though but it doesn't look like a huge improvement).

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:78
> +    bool m_dispatchDeferredEvents;

If you used a separate timer, it could act as your boolean: Timer<...>* m_deferredEventsTimer;

I think associating the suspended() states to this timer state (running vs non-running) is wrong. You could just flip the m_deferEvent / m_suspended bit after having dispatched all the pending events and clear the Timer.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:81
> +    RefPtr<Event> m_deferredProgressEvent;
> +    RefPtr<Event> m_deferredReadyStateChangeEvent;
> +    Vector<RefPtr<Event> > m_deferredEvents;

Do we get anything from this 3 types of events logic apart from a code that is a lot more complex than it should?

I also can see some ways this could break: dispatch readyState == 2, suspend the throttle until the end of the transfer and you have never dispatched readyState == 3 and that is likely to break some web-sites. (likely would need a test as we would like to ensure we don't miss a notification)

My take would be to just keep the Vector and just stash all the events there and then dispatch all of them at once. We forget the throttle after resuming but that's good enough as a first cut.

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (From update of attachment 133814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133814&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Queues events attempted dispatched while the object is suspend. Coalescing progress and
> > +        readystatechange events. Events are dispatched soon after the object has been resumed,
> > +        but not immediately to avoid hitting a recursion issue in ScriptExecutionContext.
> 
> That's why I prefer detailed ChangeLog, this paragraph could be spread below to underline the important points. Here you would keep a summary of the change and why it's needed: JS can be enabled while being suspended and the loaders don't suspend! (I *really* would like this information included as it is paramount to understanding the new design)
> 

I will expand the comments and the ChangeLog. Most of what you write below are things I should have explained better.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1152
> > +            if (!m_progressEventThrottle.suspended())
> 
> Why do we need that? That sounds like it's counter productive and would be better pushed down to the throttle.
> 
> As a side note, I just saw that suspended() is public and I think it really shouldn't.
> 
Suspended is public because it is part of the ActiveDOMObject API. The code here can be safely removed as the throttle already do handle the same thing. This is just an earlier and cheaper place to make the check.


> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:218
> > +    startOneShot(0);
> 
> I really think this should be its own Timer with a dedicated function instead of re-using the existing logic, which feels clunky.
> 
That is a good idea.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:77
> > +    bool m_deferEvents;
> 
> I am not super enthusiastic about renaming m_deferEvents to m_suspended (I could be convinced the contrary though but it doesn't look like a huge improvement).
> 
The reason is that m_deferEvents no longer represent the suspended state in the ActiveDOMObject sense. Therefore I renamed the flag to something that explicitly state what it handles.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:81
> > +    RefPtr<Event> m_deferredProgressEvent;
> > +    RefPtr<Event> m_deferredReadyStateChangeEvent;
> > +    Vector<RefPtr<Event> > m_deferredEvents;
> 
> Do we get anything from this 3 types of events logic apart from a code that is a lot more complex than it should?
> 
> I also can see some ways this could break: dispatch readyState == 2, suspend the throttle until the end of the transfer and you have never dispatched readyState == 3 and that is likely to break some web-sites. (likely would need a test as we would like to ensure we don't miss a notification)
> 
The problem is that there is no such thing as a dispatch readyState == 2. The readyStateChange events does not contain the new readyState. This means they are all 100% identical.

Therefore readystatechange events are combined so only one is send on resume. Since anything else is redundant.

[Alexey Proskuryakov]

> Suspended is public because it is part of the ActiveDOMObject API.

A function that is public in base class can be made private in a subclass when it's expected that it's only called through base class interface. In fact, it's something we strongly prefer in WebKit code base - everything that doesn't need to be public should not be.

That is a huge aid for refactoring, as you don't need to hunt down uses of every function in a class just to get the big picture of what interface it really offers to clients.

[Julien Chaffraix]

Comment on attachment 133814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133814&action=review

>>> Source/WebCore/xml/XMLHttpRequest.cpp:1152
>>> +            if (!m_progressEventThrottle.suspended())
>> 
>> Why do we need that? That sounds like it's counter productive and would be better pushed down to the throttle.
>> 
>> As a side note, I just saw that suspended() is public and I think it really shouldn't.
> 
> Suspended is public because it is part of the ActiveDOMObject API. The code here can be safely removed as the throttle already do handle the same thing. This is just an earlier and cheaper place to make the check.

XMLHttpRequest is an ActiveDOMObject, not XMLHttpRequestProgressEventThrottle. Alexei has a point too.

>>> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:81
>>> +    Vector<RefPtr<Event> > m_deferredEvents;
>> 
>> Do we get anything from this 3 types of events logic apart from a code that is a lot more complex than it should?
>> 
>> I also can see some ways this could break: dispatch readyState == 2, suspend the throttle until the end of the transfer and you have never dispatched readyState == 3 and that is likely to break some web-sites. (likely would need a test as we would like to ensure we don't miss a notification)
>> 
>> My take would be to just keep the Vector and just stash all the events there and then dispatch all of them at once. We forget the throttle after resuming but that's good enough as a first cut.
> 
> The problem is that there is no such thing as a dispatch readyState == 2. The readyStateChange events does not contain the new readyState. This means they are all 100% identical.
> 
> Therefore readystatechange events are combined so only one is send on resume. Since anything else is redundant.

Indeed, I forgot that the event doesn't have the readyState information. I fear this will break some web-developers' assumptions here but I don't see a good way of handling that.

Is there a reason why you also coalesce progress events?

Note that the downside of your approach doesn't guarantee the ordering of events due to coalescing them. I think the ordering is important to keep which is why I am pushing towards a single Vector (it also makes the logic more simple). You could easily keep track of the last readyStateChanged event and only dispatch this one.

[Allan Sandfeld Jensen]

(In reply to comment #23)
> Is there a reason why you also coalesce progress events?
> 
There entire throttle class is meant to coalesce progress events. And if they can be throttled during normal run to only be delivered with timed intervals, I thought it made best sense to keep throttling them when timed events are completely disabled.

[Allan Sandfeld Jensen]

Created attachment 135083 [details]
Patch

[Julien Chaffraix]

Comment on attachment 135083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135083&action=review

I like the new change. I would like to see the next round but the direction is very cool.

> Source/WebCore/ChangeLog:8
> +        Defers events attempted dispatched while the object is suspend. Coalescing progress and

Can we please have real English here? Usually sentences have a conjugated verb.

> Source/WebCore/ChangeLog:11
> +        to avoid hitting a recursion assert in ScriptExecutionContext.

I would like the 'why' / context to be explained as requested 2 times now. The condition of this bug got 3 people in the original bug and those *need* to appear somewhere.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:92
> +        if (m_deferredEvents.size() > 1
> +            && event->type() == eventNames().readystatechangeEvent
> +            && m_deferredEvents.last()->type() == eventNames().readystatechangeEvent)

Those should be on the same line.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:93
> +            return; // Readystatechange events are state-less so don't repeat two identical events in a row on resume.

Nit: Normally we place comment on a line by themselves. This isn't documented or particularly enforced but it arguably looks nicer. If you do change that, you need some curly braces around the statement.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:131
> +    ASSERT(m_deferEvents);

We usually check that our timer is the right timer:

ASSERT_UNUSED(timer, timer == &m_dispatchDeferredEventsTimer);

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:136
> +    Vector<RefPtr<Event> >::const_iterator it = m_deferredEvents.begin();
> +    const Vector<RefPtr<Event> >::const_iterator end = m_deferredEvents.end();
> +    for (; it != end; ++it)

We usually prefer to put the iterator |it| in the |for| loop.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:137
> +        dispatchEvent(*it);

Note that if JavaScript can interact with our associated XHR, maybe even triggering a suspend(). I really wonder if we shouldn't just swap our m_deferredEvents Vector before the loop to avoid any possible changes while iterating.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:173
> +        m_dispatchDeferredEventsTimer.stop();

We should add, in case something goes wrong:

ASSERT(m_deferEvents);

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:177
> +    ASSERT(m_deferredEvents.isEmpty());

And the matching ASSERT(!m_deferEvents) here too.

[Allan Sandfeld Jensen]

Created attachment 135545 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #26)
> (From update of attachment 135083 [details])
> I would like the 'why' / context to be explained as requested 2 times now. The condition of this bug got 3 people in the original bug and those *need* to appear somewhere.
> 
I have improved the changelog text.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:136
> > +    Vector<RefPtr<Event> >::const_iterator it = m_deferredEvents.begin();
> > +    const Vector<RefPtr<Event> >::const_iterator end = m_deferredEvents.end();
> > +    for (; it != end; ++it)
> 
> We usually prefer to put the iterator |it| in the |for| loop.
> 
I prefer them separate because long lines are much harder to read.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:137
> > +        dispatchEvent(*it);
> 
> Note that if JavaScript can interact with our associated XHR, maybe even triggering a suspend(). I really wonder if we shouldn't just swap our m_deferredEvents Vector before the loop to avoid any possible changes while iterating.
> 
Good observation. I have taken local ownership of the deferred events in the new patch.

> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:173
> > +        m_dispatchDeferredEventsTimer.stop();
> 
> We should add, in case something goes wrong:
> 
> ASSERT(m_deferEvents);
> 
> > Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:177
> > +    ASSERT(m_deferredEvents.isEmpty());
> 
> And the matching ASSERT(!m_deferEvents) here too.

Check and check.

[Julien Chaffraix]

Comment on attachment 135545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135545&action=review

> Source/WebCore/ChangeLog:17
> +

Note that you still don't explain the context here and merely describe what you are doing. As I can't get myself understood, please add the following to the ChangeLog (preferably at the beginning) before landing:

The issue is that suspending active DOM objects (which suspends the XMLHttpRequestProgressEventThrottle) doesn't stop JavaScript from running or suspend any running loader we may have. The previous code would assume those 2 cases were impossible (thus the ASSERTs triggering) and the code was modified to handle them.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:92
> +        if (m_deferredEvents.size() > 1 && event->type() == eventNames().readystatechangeEvent && event->type() == m_deferredEvents.last()->type())
> +            // Readystatechange events are state-less so avoid repeating two identical events in a row on resume.
> +            return;

There should be curly brace around that statement per our coding style.

[Allan Sandfeld Jensen]

Created attachment 137525 [details]
Patch for landing

Updated changelog as requested.

[WebKit Review Bot]

Comment on attachment 137525 [details]
Patch for landing

Clearing flags on attachment: 137525

Committed r114374: <http://trac.webkit.org/changeset/114374>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.