Bug 107954 – EventSender isn't suspended when page load is deferred

• Component: Page Loading
• Status: NEW

[Yong Li]

The timer in EventSender can still fire when the page/document is deferred (or in page cache?)

EventSender should be derived from SuspendableTimer, or at least an ActiveDOMObject.

[Yong Li]

I found this probably needs quite large refactor:

And even more scary thing is Document.cpp is calling these static functions:

    ImageLoader::dispatchPendingBeforeLoadEvents();
    ImageLoader::dispatchPendingLoadEvents();
    ImageLoader::dispatchPendingErrorEvents();

    HTMLLinkElement::dispatchPendingLoadEvents();
    HTMLStyleElement::dispatchPendingLoadEvents();

!!!!?????

[Alexey Proskuryakov]

This needs an explanation. Why does deferring loading need to suspend EventSender? If that's actually the case, it needs to be renamed appropriately, because nothing in its name says that it's not just about loading.

[Yong Li]

(In reply to comment #2)
> This needs an explanation. Why does deferring loading need to suspend EventSender? If that's actually the case, it needs to be renamed appropriately, because nothing in its name says that it's not just about loading.

Yes, it does sound a little bit confusing. However event it is just about loading, JS execution can definitely trigger some loading jobs.

From what I understand, defersLoading() means suspending loading as well as DOM activities, JS executions and other scheduled jobs that could cause re-entrancy issues or isn't always safe in that situation. A typical case is JS modal dialogs. WebKit uses PageGroupLoadDeferrer to suspend all scheduled jobs that could trigger JS execution in the nested event loop. Another case is page cache. When a frame/document is pushed into page cache, all its scheduled jobs are suspended. And the jobs will be resumed when the document/frame is restored. Doing anything to a document which is in page cache is dangerous. I'm not sure if there is any isPageCached check being done when  EventSender<> dispatches events, but even there is, we still have a problem, the events will be lost.

[Yong Li]

Created attachment 185026 [details]
Proposed solution (not for review yet)

Is this solution OK? Especially the removal of dispatching all load events in Document::implicitClose, and the removal of dispatching beforeload events from XMLDocumentParser::append(). I don't know why we need that. but there must be some reason...

[Early Warning System Bot]

Comment on attachment 185026 [details]
Proposed solution (not for review yet)

Attachment 185026 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16165668

[Early Warning System Bot]

Comment on attachment 185026 [details]
Proposed solution (not for review yet)

Attachment 185026 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16165669

[Yong Li]

Created attachment 185032 [details]
fix build

[Early Warning System Bot]

Comment on attachment 185032 [details]
fix build

Attachment 185032 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16152758

[Yong Li]

Created attachment 185035 [details]
fix build again

[Alexey Proskuryakov]

> Yes, it does sound a little bit confusing. However event it is just about loading, JS execution can definitely trigger some loading jobs.

Honestly, this is more than a little confusing to me. Personally, I would not dare to make patches in this area without deeply investigating both existing and desired behavior.

[WebKit Review Bot]

Comment on attachment 185035 [details]
fix build again

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

New failing tests:
http/tests/security/cross-frame-access-DOMImplementation.html
fast/dom/HTMLStyleElement/style-onload2.html
fast/dom/HTMLLinkElement/link-onload2.html
http/tests/security/canvas-remote-read-remote-image-allowed-with-credentials.html
fast/canvas/canvas-createPattern-fillRect-shadow.html
http/tests/security/img-with-failed-cors-check-fails-to-load.html
http/tests/security/canvas-remote-read-svg-image.html
http/tests/security/canvas-remote-read-redirect-to-remote-image.html
fast/canvas/canvas-toDataURL-webp.html
http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/canvas/canvas-drawImage-shadow.html
http/tests/security/contentSecurityPolicy/image-blocked.html
fast/dom/HTMLLinkElement/link-onload-before-page-load.html
fast/canvas/webgl/gl-teximage.html
http/tests/security/contentSecurityPolicy/image-allowed.html
http/tests/security/canvas-remote-read-data-url-svg-image.html
http/tests/security/inactive-document-with-empty-security-origin.html
fast/canvas/webgl/texture-color-profile.html

[Daniel Bates]

(In reply to comment #4)
> Created an attachment (id=185026) [details]
> Proposed solution (not for review yet)
> 
> Is this solution OK?

Clearly, this solution is not OK given the test failures mentioned in comment 11.

> Especially the removal of dispatching all load events in Document::implicitClose, and the removal of dispatching beforeload events from XMLDocumentParser::append().  I don't know why we need that. but there must be some reason...

Focusing on load events, without loss of generality, we need to dispatch all load events for <link> before we dispatch the load event for the document by <http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#the-link-element>. Similar arguments can be made for <img> and <style> as well as the WebKit-specific beforeload event (*).

(*) See <http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/MessagesandProxies/MessagesandProxies.html#//apple_ref/doc/uid/TP40009977-CH14-SW9> for more details.

[Yong Li]

(In reply to comment #10)
> > Yes, it does sound a little bit confusing. However event it is just about loading, JS execution can definitely trigger some loading jobs.
> 
> Honestly, this is more than a little confusing to me. Personally, I would not dare to make patches in this area without deeply investigating both existing and desired behavior.

We already have a lot of code (like ActiveDOMObjects, SuspendableTimer, DocumentEventQueue and other classes/functions) that suspends and resumes scheduled timers accordingly.

I think the existing behavior for these load events is:

1) schedule event dispatching with timer. Dispatch events when the timer fires
2) Force dispatching the events in Document.implicitClose() and XMLDocumentParser::append()

I wouldn't change the behavior. My proposed patch misses 2), which I think can be done by keeping the event sender lists in the document object (rather than using global static lists).

Thanks Dan. I'll also try to make a test case even a manual one to show the JS re-entrancy when time allowing.