Changeset 192844 in webkit


Ignore:
Timestamp:
Nov 30, 2015 4:33:47 PM (8 years ago)
Author:
jiewen_tan@apple.com
Message:

Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
https://bugs.webkit.org/show_bug.cgi?id=149309
<rdar://problem/22748363>

Reviewed by Brent Fulgham.

Source/WebCore:

A weird order of event execution introduced by the test case will kill the webpage in a
subframe of the page while executing its |frame.loader().checkLoadCompleteForThisFrame()|.
Therefore, any frames comes after the failing subframe will have no page. Check it before
calling to those frames' |frame.loader().checkLoadCompleteForThisFrame()|, otherwise the
assertion in |frame.loader().checkLoadCompleteForThisFrame()| will fail.

Test: http/tests/misc/detach-during-notifyDone.html

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::checkLoadComplete):

Source/WebKit/mac:

  • WebView/WebDataSource.mm:

(WebDataSourcePrivate::~WebDataSourcePrivate):
Refine the assertion to treat <rdar://problem/9673866>.

Source/WebKit2:

Callback of bundle clients could kill the documentloader. Therefore, make a copy
of the navigationID before invoking the callback.

  • WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

(WebKit::WebFrameLoaderClient::dispatchDidChangeLocationWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidPushStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidReplaceStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidPopStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidFailLoad):
(WebKit::WebFrameLoaderClient::dispatchDidFinishDocumentLoad):
(WebKit::WebFrameLoaderClient::dispatchDidFinishLoad):

LayoutTests:

The test case is from Blink r175601:
https://codereview.chromium.org/317513002
The test case will generate a set of weird ordering events that affects the documentLoader:

  1. The subframe finishes loading, and since the frame’s testRunner is not set to wait until

done, WebKitTestRunner stops the load (by calling WKBundlePageStopLoading()).

  1. This causes the in-progress XHR to be aborted, which causes its readyState to become DONE

(this bug doesn’t always reproduce because sometimes the XHR has already finished before the
frame finishes loading).

  1. The onreadystatechange callback is executed, which sets innerHTML on the parent frame.
  2. Setting innerHTML disconnects the subframe, nulling out its DocumentLoader.
  3. We return to WebFrameLoaderClient::dispatchDidFinishLoad() from step #1, but now the

FrameLoader’s DocumentLoader is null. And WebKit crashes here.

Note that steps 2-4 happen synchronously inside WebFrameLoaderClient::dispatchDidFinishLoad().

  • http/tests/misc/detach-during-notifyDone-expected.txt: Added.
  • http/tests/misc/detach-during-notifyDone.html: Added.
  • http/tests/misc/resources/detached-frame.html: Added.
Location:
trunk
Files:
3 added
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r192843 r192844  
     12015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149309
     5        <rdar://problem/22748363>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        The test case is from Blink r175601:
     10        https://codereview.chromium.org/317513002
     11        The test case will generate a set of weird ordering events that affects the documentLoader:
     12        1. The subframe finishes loading, and since the frame’s testRunner is not set to wait until
     13        done, WebKitTestRunner stops the load (by calling WKBundlePageStopLoading()).
     14        2. This causes the in-progress XHR to be aborted, which causes its readyState to become DONE
     15        (this bug doesn’t always reproduce because sometimes the XHR has already finished before the
     16        frame finishes loading).
     17        3. The onreadystatechange callback is executed, which sets innerHTML on the parent frame.
     18        4. Setting innerHTML disconnects the subframe, nulling out its DocumentLoader.
     19        5. We return to WebFrameLoaderClient::dispatchDidFinishLoad() from step #1, but now the
     20        FrameLoader’s DocumentLoader is null. And WebKit crashes here.
     21
     22        Note that steps 2-4 happen synchronously inside WebFrameLoaderClient::dispatchDidFinishLoad().
     23
     24        * http/tests/misc/detach-during-notifyDone-expected.txt: Added.
     25        * http/tests/misc/detach-during-notifyDone.html: Added.
     26        * http/tests/misc/resources/detached-frame.html: Added.
     27
    1282015-11-30  Commit Queue  <commit-queue@webkit.org>
    229
  • trunk/Source/WebCore/ChangeLog

    r192843 r192844  
     12015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149309
     5        <rdar://problem/22748363>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        A weird order of event execution introduced by the test case will kill the webpage in a
     10        subframe of the page while executing its |frame.loader().checkLoadCompleteForThisFrame()|.
     11        Therefore, any frames comes after the failing subframe will have no page. Check it before
     12        calling to those frames' |frame.loader().checkLoadCompleteForThisFrame()|, otherwise the
     13        assertion in |frame.loader().checkLoadCompleteForThisFrame()| will fail.
     14
     15        Test: http/tests/misc/detach-during-notifyDone.html
     16
     17        * loader/FrameLoader.cpp:
     18        (WebCore::FrameLoader::checkLoadComplete):
     19
    1202015-11-30  Commit Queue  <commit-queue@webkit.org>
    221
  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r192711 r192844  
    24322432
    24332433    // To process children before their parents, iterate the vector backwards.
    2434     for (unsigned i = frames.size(); i; --i)
    2435         frames[i - 1]->loader().checkLoadCompleteForThisFrame();
     2434    for (auto frame = frames.rbegin(); frame != frames.rend(); ++frame) {
     2435        if ((*frame)->page())
     2436            (*frame)->loader().checkLoadCompleteForThisFrame();
     2437    }
    24362438}
    24372439
  • trunk/Source/WebKit/mac/ChangeLog

    r192725 r192844  
     12015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149309
     5        <rdar://problem/22748363>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        * WebView/WebDataSource.mm:
     10        (WebDataSourcePrivate::~WebDataSourcePrivate):
     11        Refine the assertion to treat <rdar://problem/9673866>.
     12
    113== Rolled over to ChangeLog-2015-11-21 ==
  • trunk/Source/WebKit/mac/WebView/WebDataSource.mm

    r184853 r192844  
    9393    {
    9494        if (loader) {
    95             ASSERT(!loader->isLoading());
     95            // We might run in to infinite recursion if we're stopping loading as the result of detaching from the frame.
     96            // Therefore, DocumentLoader::detachFromFrame() did some smart things to stop the recursion.
     97            // As a result of breaking the resursion, DocumentLoader::m_subresourceLoader
     98            // and DocumentLoader::m_plugInStreamLoaders might not be empty at this time.
     99            // See <rdar://problem/9673866> for more details.
     100            ASSERT(!loader->isLoading() || loader->isStopping());
    96101            loader->detachDataSource();
    97102        }
  • trunk/Source/WebKit2/ChangeLog

    r192834 r192844  
     12015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149309
     5        <rdar://problem/22748363>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        Callback of bundle clients could kill the documentloader. Therefore, make a copy
     10        of the navigationID before invoking the callback.
     11
     12        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     13        (WebKit::WebFrameLoaderClient::dispatchDidChangeLocationWithinPage):
     14        (WebKit::WebFrameLoaderClient::dispatchDidPushStateWithinPage):
     15        (WebKit::WebFrameLoaderClient::dispatchDidReplaceStateWithinPage):
     16        (WebKit::WebFrameLoaderClient::dispatchDidPopStateWithinPage):
     17        (WebKit::WebFrameLoaderClient::dispatchDidFailLoad):
     18        (WebKit::WebFrameLoaderClient::dispatchDidFinishDocumentLoad):
     19        (WebKit::WebFrameLoaderClient::dispatchDidFinishLoad):
     20
    1212015-11-30  Tim Horton  <timothy_horton@apple.com>
    222
  • trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r192601 r192844  
    328328    RefPtr<API::Object> userData;
    329329
     330    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     331
    330332    // Notify the bundle client.
    331333    webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationAnchorNavigation, userData);
    332334
    333335    // Notify the UIProcess.
    334     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    335     webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationAnchorNavigation, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     336    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationAnchorNavigation, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    336337}
    337338
     
    344345    RefPtr<API::Object> userData;
    345346
     347    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     348
    346349    // Notify the bundle client.
    347350    webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStatePush, userData);
    348351
    349352    // Notify the UIProcess.
    350     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    351     webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStatePush, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     353    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStatePush, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    352354}
    353355
     
    360362    RefPtr<API::Object> userData;
    361363
     364    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     365
    362366    // Notify the bundle client.
    363367    webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStateReplace, userData);
    364368
    365369    // Notify the UIProcess.
    366     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    367     webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStateReplace, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     370    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStateReplace, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    368371}
    369372
     
    376379    RefPtr<API::Object> userData;
    377380
     381    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     382
    378383    // Notify the bundle client.
    379384    webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStatePop, userData);
    380385
    381386    // Notify the UIProcess.
    382     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    383     webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStatePop, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     387    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStatePop, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    384388}
    385389
     
    505509    RefPtr<API::Object> userData;
    506510
     511    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     512
    507513    // Notify the bundle client.
    508514    webPage->injectedBundleLoaderClient().didFailLoadWithErrorForFrame(webPage, m_frame, error, userData);
    509515
    510516    // Notify the UIProcess.
    511     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    512     webPage->send(Messages::WebPageProxy::DidFailLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), error, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     517    webPage->send(Messages::WebPageProxy::DidFailLoadForFrame(m_frame->frameID(), navigationID, error, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    513518
    514519    // If we have a load listener, notify it.
     
    525530    RefPtr<API::Object> userData;
    526531
     532    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     533
    527534    // Notify the bundle client.
    528535    webPage->injectedBundleLoaderClient().didFinishDocumentLoadForFrame(webPage, m_frame, userData);
    529536
    530     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    531 
    532537    // Notify the UIProcess.
    533     webPage->send(Messages::WebPageProxy::DidFinishDocumentLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     538    webPage->send(Messages::WebPageProxy::DidFinishDocumentLoadForFrame(m_frame->frameID(), navigationID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    534539}
    535540
     
    542547    RefPtr<API::Object> userData;
    543548
     549    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
     550
    544551    // Notify the bundle client.
    545552    webPage->injectedBundleLoaderClient().didFinishLoadForFrame(webPage, m_frame, userData);
    546553
    547     WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
    548 
    549554    // Notify the UIProcess.
    550     webPage->send(Messages::WebPageProxy::DidFinishLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
     555    webPage->send(Messages::WebPageProxy::DidFinishLoadForFrame(m_frame->frameID(), navigationID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
    551556
    552557    // If we have a load listener, notify it.
Note: See TracChangeset for help on using the changeset viewer.