[webkit-reviews] review denied: [Bug 123828] AX: .js dialogs shown in unload while AX is running crash WebKit. : [Attachment 218549] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 123828: AX: .js dialogs shown in unload while AX is running crash WebKit.
https://bugs.webkit.org/show_bug.cgi?id=123828

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218549&action=review


I’m not sure this concept is going to work. The code in
SubresourceLoader::didFinishLoading seems to only scratch the surface in terms
of the number of bad things that could happen during the “stall for JavaScript
alerts” state; we’d need something more complete and robust, maybe at a lower
level?

And adding a separate timer to each subresource also seems wrong. This is an
overall state that I think is browser-context-wide, so this deferral mechanism
should probably be tied to the entire browsing context, not a single sub
resource. It needs to be a lot more like the existing setDefersLoading
mechanism, which has a similar (maybe identical?) goal.

> Source/WebCore/loader/SubresourceLoader.cpp:83
> +    m_loadingCompleteRetryTimer.stop();

This line of code is not needed or helpful. A timer that is destroyed stops
itself, and since this is the destructor the timer that is a data member will
be destroyed.

> Source/WebCore/loader/SubresourceLoader.cpp:277
> +    if (m_documentLoader->frameLoader()->pageDismissalEventBeingDispatched()
== FrameLoader::UnloadDismissal &&
!m_documentLoader->frameLoader()->unloadEventEmitted()) {

This is a fragile rule, but it makes it even worse to have to check these
things directly. I think we need a function on frame loader that encapsulates
this into a single boolean.

But what about when this is one frame, and the unload event is being processed
in another frame elsewhere? It seems to me that we shouldn’t do this processing
on any frame if any frame is in this state, since JavaScript could reach over
to other frames and interact with them. I think this check is too narrow, and
potentially needed in *many* other places.

> Source/WebCore/loader/SubresourceLoader.cpp:279
> +	       m_loadingCompleteRetryTimer.startOneShot(0.2);

Where does this 0.2 second duration come from? I’d prefer a named constant at
the top of the file with a brief comment explaining the rational.