Bug 107954

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(); !!!!?????

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.

(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.

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...

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

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

Created attachment 185032 [details] fix build

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

Created attachment 185035 [details] fix build again

> 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.

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

(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.

(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.