[webkit-reviews] review canceled: [Bug 81506] Asserts in XMLHttpRequestProgressEventThrottle : [Attachment 132811] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 81506: Asserts in XMLHttpRequestProgressEventThrottle
https://bugs.webkit.org/show_bug.cgi?id=81506

Attachment 132811: Patch
https://bugs.webkit.org/attachment.cgi?id=132811&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.