[webkit-reviews] review canceled: [Bug 81506] Asserts in XMLHttpRequestProgressEventThrottle : [Attachment 133814] 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 133814: Patch
https://bugs.webkit.org/attachment.cgi?id=133814&action=review

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