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

• Component: Page Loading
• Status: ASSIGNED

[Brady Eidson]

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>

[Brady Eidson]

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]

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]

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]

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]

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

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

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

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

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]

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]

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

[Brady Eidson]

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

[Brady Eidson]

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

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]

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

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

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

[Brady Eidson]

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

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