Bug 84488 - Extra work done by PageGroupLoadDeferrer besides Page::setDefersLoading needs to be done by Page::setDefersLoading
Summary: Extra work done by PageGroupLoadDeferrer besides Page::setDefersLoading needs...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on: 84490
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-20 13:43 PDT by Brady Eidson
Modified: 2014-08-06 22:12 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 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>
Comment 1 Brady Eidson 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.
Comment 2 Brady Eidson 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.
Comment 3 Brady Eidson 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.
Comment 4 Yong Li 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);
}
Comment 5 Brady Eidson 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()
Comment 6 Brady Eidson 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.
Comment 7 Brady Eidson 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.
Comment 8 Brady Eidson 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.
Comment 9 Brady Eidson 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.
Comment 10 Brady Eidson 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.
Comment 11 Brady Eidson 2012-04-20 17:08:10 PDT
Renamed WillShowDialog to WillDeferLoading in http://trac.webkit.org/changeset/114802
Comment 12 Brady Eidson 2012-04-20 17:48:05 PDT
Created attachment 138207 [details]
Patch v1 - Fold the extra work in to the common method
Comment 13 Brady Eidson 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.
Comment 14 Yong Li 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)
Comment 15 Brady Eidson 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...
Comment 16 Brady Eidson 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.
Comment 17 Brady Eidson 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.
Comment 18 Darin Adler 2012-04-23 15:11:45 PDT
We may need to roll out r102278 if we aren’t able to resolve this soon.
Comment 19 Brady Eidson 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.
Comment 20 Daniel Bates 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.