Bug 18288 – FrameLoader wrongly assumes dispatchDecidePolicyForNavigationAction is synchronous

• Status: NEW
• Component: New Bugs

[Eric Seidel (no email)]

FrameLoader wrongly assumes dispatchDecidePolicyForNavigationAction is synchronous

I was browsing around in our frame loader today and saw this:

    m_delegateIsDecidingNavigationPolicy = true;
    m_client->dispatchDecidePolicyForNavigationAction(&FrameLoader::continueAfterNavigationPolicy,
        action, request);
    m_delegateIsDecidingNavigationPolicy = false;

That looks wrong.  Looking at dispatchDecidePolicyForNavigationAction, which calls: (up in webkit)

    [[webView _policyDelegateForwarder] webView:webView
                decidePolicyForNavigationAction:actionDictionary(action)
                                        request:request.nsURLRequest()
                                          frame:m_webFrame.get()
                               decisionListener:setUpPolicyListener(function).get()];

Assumes that the policy delegate makes a synchronous call to the policy listener from within this decidePolicyForNavigationAction: call.

This seems like a bogus assumption.  At least if your policy delegate needs to do any work to decide if the navigation should happen.

It looks like the only consequence of this error is that:
bool FrameLoader::shouldReloadToHandleUnreachableURL(DocumentLoader* docLoader)

may return the wrong value, causing:

FrameLoader::load() to screw up the b/f list:

    // When we loading alternate content for an unreachable URL that we're
    // visiting in the b/f list, we treat it as a reload so the b/f list 
    // is appropriately maintained.
    if (shouldReloadToHandleUnreachableURL(newDocumentLoader)) {
        ASSERT(type == FrameLoadTypeStandard);
        type = FrameLoadTypeReload;
    }

Again, I don't have any test case which breaks this.  This is not a bug in Safari.  This is only ever a bug if some WebKit client were to implement dispatchDecidePolicyForNavigationAction delegate method asynchronous.  But the code looked wrong (and further inspection seemed to confirm the wrongness), so I'm filing this bug.