Bug 84488

Summary:

Extra work done by PageGroupLoadDeferrer besides Page::setDefersLoading needs to be done by Page::setDefersLoading

Product:

WebKit

Reporter:

Brady Eidson <beidson>

Component:

Page Loading

Assignee:

Brady Eidson <beidson>

Status:

ASSIGNED

Severity:

Normal

CC:

ap, darin, dbates, jchaffraix, psolanki, yong.li.webkit

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

84490

Bug Blocks:

Attachments:

Description	Flags
Patch v1 - Fold the extra work in to the common method	beidson: review-

Brady Eidson

Reported 2012-04-20 13:43:03 PDT

Extra work done by PageGroupLoadDeferrer besides Page::setDefersLoading needs to be done by Page::setDefersLoading At least one platform (Mac) exposes Page::setDefersLoading in their API. PageGroupLoadDeferrer does extra work that is important for deferring loading but that extra work is skipped when APIs defer loading directly. To further exacerbate this problem at least some sections of the code conflate "page->defersLoading()" with "PageGroupLoadDeferrer's extra work has been done". One example is Document::didReceiveTask. If page->defersLoading() is true, it throws the task on a pending queue. But that queue is only flushed when a PageGroupLoadDeferrer is destroyed, and not when the state of page->defersLoading() otherwise changes. This was introduced in 102278 and caused a lot of bizarre little side effects. --- One complication in this cleanup will addressing this long-time comment in PageGroupLoadDefererr: // NOTE: if PageGroupLoadDeferrer is ever used for tasks other than showing a modal window or sheet, // the constructor will need to take a ActiveDOMObject::ReasonForSuspension. The time for addressing that is overdue! In radar as <rdar://problem/10484294>

Attachments
Patch v1 - Fold the extra work in to the common method (4.82 KB, patch) 2012-04-20 17:48 PDT, Brady Eidson	beidson: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Brady Eidson

Comment 1 2012-04-20 14:30:15 PDT

Fixed the comment in https://bugs.webkit.org/show_bug.cgi?id=84490 and http://trac.webkit.org/changeset/114782 Now on to the real work.

Brady Eidson

Comment 2 2012-04-20 14:33:22 PDT

Another interesting thing about this work was that the extra PageGroupLoadDeferrer work didn't respect m_settings->loadDeferringEnabled() on a page-by-page basis. It should have.

Brady Eidson

Comment 3 2012-04-20 14:50:27 PDT

As I started working on a patch, it became apparent that: >To further exacerbate this problem at least some sections of the code conflate "page->defersLoading()" with "PageGroupLoadDeferrer's extra work has been done". Is actually the key problem behind <rdar://problem/10484294>. While this bug is still very real and should remain open, a larger overhaul might not be necessary right now... back with more soon.

Yong Li

Comment 4 2012-04-20 14:52:39 PDT

Do you know why Page::setDefersLoading() is used instead of PageGroupLoadDeferrer? Everyone should use PageGroupLoadDeferrer from my point of view, because the order of destructing them is critical. Be careful with script debugger. They are doing partial defer work, too. void PageScriptDebugServer::setJavaScriptPaused(Frame* frame, bool paused) { ASSERT_ARG(frame, frame); if (!frame->script()->canExecuteScripts(NotAboutToExecuteScript)) return; frame->script()->setPaused(paused); Document* document = frame->document(); if (paused) { document->suspendScriptedAnimationControllerCallbacks(); document->suspendActiveDOMObjects(ActiveDOMObject::JavaScriptDebuggerPaused); } else { document->resumeActiveDOMObjects(); document->resumeScriptedAnimationControllerCallbacks(); } setJavaScriptPaused(frame->view(), paused); }

Brady Eidson

Comment 5 2012-04-20 14:59:38 PDT

(In reply to comment #3) > As I started working on a patch, it became apparent that: > > >To further exacerbate this problem at least some sections of the code conflate "page->defersLoading()" with "PageGroupLoadDeferrer's extra work has been done". > > Is actually the key problem behind <rdar://problem/10484294>. It's the first half of the problem. The second half of the problem is that when Page::setDefersLoading(false) is called no one tells the Document to resume performing its callbacks. I think the "extra" code really does need to be moved into the active part of Page::setDefersLoading()

Brady Eidson

Comment 6 2012-04-20 15:03:59 PDT

(In reply to comment #4) > Do you know why Page::setDefersLoading() is used instead of PageGroupLoadDeferrer? Everyone should use PageGroupLoadDeferrer from my point of view, because the order of destructing them is critical. Yes... because it is Mac API that has existed since the beginning of the WebKit project, and Cocoa clients use it. PageGroupLoadDeferrer was added much later as a "temporary load deferring locker" meant to have stack lifetime. But by the time it was added Cocoa clients had already established a precedent of deferring/undeferring on their own terms. The moment PageGroupLoadDeferrer did any additional work besides calling Page::setDefersLoading(), that was a bug introduced to Cocoa clients. Such are the concerns of maintaining a widely used API... > Be careful with script debugger. They are doing partial defer work, too. > > void PageScriptDebugServer::setJavaScriptPaused(Frame* frame, bool paused) > { > ASSERT_ARG(frame, frame); > > if (!frame->script()->canExecuteScripts(NotAboutToExecuteScript)) > return; > > frame->script()->setPaused(paused); > > Document* document = frame->document(); > if (paused) { > document->suspendScriptedAnimationControllerCallbacks(); > document->suspendActiveDOMObjects(ActiveDOMObject::JavaScriptDebuggerPaused); > } else { > document->resumeActiveDOMObjects(); > document->resumeScriptedAnimationControllerCallbacks(); > } > > setJavaScriptPaused(frame->view(), paused); > } This is not a concern as these are all mechanisms separate from load deferring, and are not exposed as API. The crux of the problem was introducing two different load deferring behaviors.

Brady Eidson

Comment 7 2012-04-20 15:21:20 PDT

(In reply to comment #6) > (In reply to comment #4) > > Do you know why Page::setDefersLoading() is used instead of PageGroupLoadDeferrer? Everyone should use PageGroupLoadDeferrer from my point of view, because the order of destructing them is critical. > > Yes... because it is Mac API that has existed since the beginning of the WebKit project, and Cocoa clients use it. And another key reason is that they defer loads on a per-Page basis instead of a per-PageGroup basis.

Brady Eidson

Comment 8 2012-04-20 15:24:19 PDT

(In reply to comment #6) > (In reply to comment #4) > > Do you know why Page::setDefersLoading() is used instead of PageGroupLoadDeferrer? Everyone should use PageGroupLoadDeferrer from my point of view, because the order of destructing them is critical. On a per-page basis, it shouldn't be difficult to preserve the order of operations that PageGroupLoadDeferrer currently enforces. But if your assertion is that it is important that all of the document scheduled task suspension must take place before all of the actual calls to setDefersLoading... I would have to ask for clarification as to why.

Brady Eidson

Comment 9 2012-04-20 16:29:48 PDT

All of these different ActiveDOMObject::ReasonForSuspension cases are amusing. If you grep the project you find that precisely *ONE* ActiveDOMObject (HTMLMediaElement) behaves differently based on only *one* of the flags. In other words, WillShowDialog carries absolutely zero special significance and can be renamed to "WillDeferLoading" then used generically in all load deferring code paths.

Brady Eidson

Comment 10 2012-04-20 16:31:54 PDT

Which means that much of https://bugs.webkit.org/show_bug.cgi?id=84490 and http://trac.webkit.org/changeset/114782 were unnecessary (though the Document parts still were) But as a logical step forward it was still a good exploration.

Brady Eidson

Comment 11 2012-04-20 17:08:10 PDT

Renamed WillShowDialog to WillDeferLoading in http://trac.webkit.org/changeset/114802

Brady Eidson

Comment 12 2012-04-20 17:48:05 PDT

Created attachment 138207 [details] Patch v1 - Fold the extra work in to the common method

Brady Eidson

Comment 13 2012-04-20 17:49:07 PDT

(In reply to comment #12) > Created an attachment (id=138207) [details] > Patch v1 - Fold the extra work in to the common method Realistically I'd be surprised if this changed behavior for WebCore or any other WebKit clients that haven't exposed setDefersLoading directly. But Yong had some concerns, so here is my proposed patch for him to look at.

Yong Li

Comment 14 2012-04-23 08:18:30 PDT

Brady, I tried doing the similar change before. But it caused assert failures because of conflicts with other calls to page->setDefersLoading(). If the API must call setDefersLoading() by themselves, we probably should make m_defersLoading a counter. Otherwise, the following senario could happen: Executing JS ... To show a modal dialog setDefersLoading(true) (by PageGroupLoadDeferrer) ... setDefersLoading(true) (by API) ... setDefersLoading(false) (by API) ... (!!!tasks are assumed here!!!) ... Executing JS when showing modal dialog. Re-entrancy ... setDefersLoading(false) (by PageGroupLoadDeferrer)

Brady Eidson

Comment 15 2012-04-23 09:20:48 PDT

(In reply to comment #14) > > If the API must call setDefersLoading() by themselves, They already do. We must continue supporting it. And there's already real world breakage with a long standing Apple internal project, and fixing that is my primary task right now. > Brady, I tried doing the similar change before. But it caused assert failures because of conflicts with other calls to page->setDefersLoading(). Can you provide a concrete example of where this happened, and what the backtraces were? > we probably should make m_defersLoading a counter. Otherwise, the following senario could happen: > > Executing JS > ... > To show a modal dialog > setDefersLoading(true) (by PageGroupLoadDeferrer) > ... > setDefersLoading(true) (by API) > ... > setDefersLoading(false) (by API) > ... > (!!!tasks are assumed here!!!) > ... > Executing JS when showing modal dialog. Re-entrancy > ... > setDefersLoading(false) (by PageGroupLoadDeferrer) I agree it should be a counter... In addition to the theoretical bug of Mac WebKit API users of setDefersLoading() interacting poorly with PageGroupLoadDeferrers, there's already an obviously real bug in WebKit/chromium in that they use a stack of PageGroupLoadDeferrers for entering/exiting modal loops. I do not think the stack behaves the way they think it behaves...

Brady Eidson

Comment 16 2012-04-23 09:24:43 PDT

Just as a heads up: The current state of things is very broken and sorting it out is very high priority for us. I fully respect that you've done the most recent work in this area and hope to keep getting your feedback, but I have to be moving forward quickly on this and code changes will need to be happening very soon.

Brady Eidson

Comment 17 2012-04-23 10:21:54 PDT

Comment on attachment 138207 [details] Patch v1 - Fold the extra work in to the common method Running layout tests shows inspector failures related to load deferring. Now that I have firms examples of the asserts Yong alluded too, I'll work on other approaches.

Darin Adler

Comment 18 2012-04-23 15:11:45 PDT

We may need to roll out r102278 if we aren’t able to resolve this soon.

Brady Eidson

Comment 19 2012-04-23 15:45:16 PDT

(In reply to comment #18) > We may need to roll out r102278 if we aren’t able to resolve this soon. I found another way to resolve the radar <rdar://problem/10484294> that makes figuring this out in WebKit less pressing. But this bug should remain open to track the work that still needs to be done for future sanities sake.

Daniel Bates

Comment 20 2014-08-06 22:12:12 PDT

(In reply to comment #0) > One example is Document::didReceiveTask. If page->defersLoading() is true, it throws the task on a pending queue. But that queue is only flushed when a PageGroupLoadDeferrer is destroyed, and not when the state of page->defersLoading() otherwise changes. This was introduced in 102278 and caused a lot of bizarre little side effects. I broke out a fix for this issue into bug #135688 from the larger refactoring effort proposed by this bug.

Note You need to log in before you can comment on or make changes to this bug.