Changeset 129910 in webkit


Ignore:
Timestamp:
Sep 28, 2012, 9:38:08 AM (13 years ago)
Author:
leandrogracia@chromium.org
Message:

[Chromium] Fix the find-in-page implementation for detaching frames.
​https://bugs.webkit.org/show_bug.cgi?id=97807

Reviewed by Adam Barth.

Follow-up of 97688. Introduces proper test coverage for the find-in-page
feature in detaching/detached frame situations, fixing a few crashes and
ensuring that a final reply is always sent.

  • public/WebNode.h:
  • src/WebFrameImpl.cpp:

(WebKit::WebFrameImpl::find):
(WebKit::WebFrameImpl::scopeStringMatches):
(WebKit::WebFrameImpl::flushCurrentScopingEffort):
(WebKit):
(WebKit::WebFrameImpl::finishCurrentScopingEffort):
(WebKit::WebFrameImpl::cancelPendingScopingEffort):
(WebKit::WebFrameImpl::WebFrameImpl):
(WebKit::WebFrameImpl::setWebCoreFrame):
(WebKit::WebFrameImpl::initializeAsMainFrame):
(WebKit::WebFrameImpl::createChildFrame):
(WebKit::WebFrameImpl::shouldScopeMatches):
(WebKit::WebFrameImpl::willDetachPage):

  • src/WebFrameImpl.h:

(WebFrameImpl):

  • src/WebNode.cpp:

(WebKit::WebNode::remove):
(WebKit):

  • tests/WebFrameTest.cpp:
  • tests/data/find_in_page.html:
Location:
trunk/Source/WebKit/chromium
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/chromium/ChangeLog

    r129903 r129910  
     12012-09-28  Leandro Gracia Gil  <leandrogracia@chromium.org>
     2
     3        [Chromium] Fix the find-in-page implementation for detaching frames.
     4        https://bugs.webkit.org/show_bug.cgi?id=97807
     5
     6        Reviewed by Adam Barth.
     7
     8        Follow-up of 97688. Introduces proper test coverage for the find-in-page
     9        feature in detaching/detached frame situations, fixing a few crashes and
     10        ensuring that a final reply is always sent.
     11
     12        * public/WebNode.h:
     13        * src/WebFrameImpl.cpp:
     14        (WebKit::WebFrameImpl::find):
     15        (WebKit::WebFrameImpl::scopeStringMatches):
     16        (WebKit::WebFrameImpl::flushCurrentScopingEffort):
     17        (WebKit):
     18        (WebKit::WebFrameImpl::finishCurrentScopingEffort):
     19        (WebKit::WebFrameImpl::cancelPendingScopingEffort):
     20        (WebKit::WebFrameImpl::WebFrameImpl):
     21        (WebKit::WebFrameImpl::setWebCoreFrame):
     22        (WebKit::WebFrameImpl::initializeAsMainFrame):
     23        (WebKit::WebFrameImpl::createChildFrame):
     24        (WebKit::WebFrameImpl::shouldScopeMatches):
     25        (WebKit::WebFrameImpl::willDetachPage):
     26        * src/WebFrameImpl.h:
     27        (WebFrameImpl):
     28        * src/WebNode.cpp:
     29        (WebKit::WebNode::remove):
     30        (WebKit):
     31        * tests/WebFrameTest.cpp:
     32        * tests/data/find_in_page.html:
     33
    1342012-09-28  Kent Tamura  <tkent@chromium.org>
    235

  • trunk/Source/WebKit/chromium/public/WebNode.h

    r124790 r129910  
    114114    WEBKIT_EXPORT WebElement rootEditableElement() const;
    115115    WEBKIT_EXPORT bool focused() const;
     116    WEBKIT_EXPORT bool remove();
    116117
    117118    // Returns true if the node has a non-empty bounding box in layout.

  • trunk/Source/WebKit/chromium/src/WebFrameImpl.cpp

    r129850 r129910  
    526526}
    527527
    528 // WebFrame -------------------------------------------------------------------
    529 
    530528class WebFrameImpl::DeferredScopeStringMatches {
    531529public:
    … …  
    559557    bool m_reset;
    560558};
    561 
    562559
    563560// WebFrame -------------------------------------------------------------------
    … …  
    16191616                        WebRect* selectionRect)
    16201617{
     1618    if (!frame() || !frame()->page())
     1619        return false;
     1620
    16211621    WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
    16221622
    … …  
    17171717                                      bool reset)
    17181718{
    1719     WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
    1720 
    17211719    if (reset) {
    17221720        // This is a brand new search, so we need to reset everything.
    17231721        // Scoping is just about to begin.
    1724         m_scopingComplete = false;
     1722        m_scopingInProgress = true;
     1723
     1724        // Need to keep the current identifier locally in order to finish the
     1725        // request in case the frame is detached during the process.
     1726        m_findRequestIdentifier = identifier;
    17251727
    17261728        // Clear highlighting for this frame.
    … …  
    17371739        m_resumeScopingFromRange = 0;
    17381740
    1739         mainFrameImpl->m_framesScopingCount++;
     1741        // The view might be null on detached frames.
     1742        if (frame() && frame()->page())
     1743            viewImpl()->mainFrameImpl()->m_framesScopingCount++;
    17401744
    17411745        // Now, defer scoping until later to allow find operation to finish quickly.
    … …  
    17561760    }
    17571761
     1762    WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
    17581763    RefPtr<Range> searchRange(rangeOfContents(frame()->document()));
    17591764
    … …  
    18851890}
    18861891
    1887 void WebFrameImpl::finishCurrentScopingEffort(int identifier)
    1888 {
     1892void WebFrameImpl::flushCurrentScopingEffort(int identifier)
     1893{
     1894    if (!frame() || !frame()->page())
     1895        return;
     1896
    18891897    WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
    18901898
    18911899    // This frame has no further scoping left, so it is done. Other frames might,
    18921900    // of course, continue to scope matches.
    1893     m_scopingComplete = true;
    18941901    mainFrameImpl->m_framesScopingCount--;
    1895     m_lastFindRequestCompletedWithNoMatches = !m_lastMatchCount;
    18961902
    18971903    // If this is the last frame to finish scoping we need to trigger the final
    … …  
    18991905    if (!mainFrameImpl->m_framesScopingCount)
    19001906        mainFrameImpl->increaseMatchCount(0, identifier);
     1907}
     1908
     1909void WebFrameImpl::finishCurrentScopingEffort(int identifier)
     1910{
     1911    flushCurrentScopingEffort(identifier);
     1912
     1913    m_scopingInProgress = false;
     1914    m_lastFindRequestCompletedWithNoMatches = !m_lastMatchCount;
    19011915
    19021916    // This frame is done, so show any scrollbar tickmarks we haven't drawn yet.
    … …  
    19111925    m_activeMatchIndexInCurrentFrame = -1;
    19121926
    1913     if (!m_scopingComplete)
     1927    // Last request didn't complete.
     1928    if (m_scopingInProgress)
    19141929        m_lastFindRequestCompletedWithNoMatches = false;
     1930
     1931    m_scopingInProgress = false;
    19151932}
    19161933
    … …  
    22742291
    22752292WebFrameImpl::WebFrameImpl(WebFrameClient* client)
    2276     : m_frameLoaderClient(this)
     2293    : FrameDestructionObserver(0)
     2294    , m_frameLoaderClient(this)
    22772295    , m_client(client)
    22782296    , m_frame(0)
    … …  
    22842302    , m_totalMatchCount(-1)
    22852303    , m_framesScopingCount(-1)
    2286     , m_scopingComplete(false)
     2304    , m_findRequestIdentifier(-1)
     2305    , m_scopingInProgress(false)
    22872306    , m_lastFindRequestCompletedWithNoMatches(false)
    22882307    , m_nextInvalidateAfter(0)
    … …  
    23052324}
    23062325
     2326void WebFrameImpl::setWebCoreFrame(WebCore::Frame* frame)
     2327{
     2328    ASSERT(frame);
     2329    m_frame = frame;
     2330    observeFrame(m_frame);
     2331}
     2332
    23072333void WebFrameImpl::initializeAsMainFrame(WebCore::Page* page)
    23082334{
    23092335    RefPtr<Frame> frame = Frame::create(page, 0, &m_frameLoaderClient);
    2310     m_frame = frame.get();
     2336    setWebCoreFrame(frame.get());
    23112337
    23122338    // Add reference on behalf of FrameLoader.  See comments in
    … …  
    23312357    RefPtr<Frame> childFrame = Frame::create(
    23322358        m_frame->page(), ownerElement, &webframe->m_frameLoaderClient);
    2333     webframe->m_frame = childFrame.get();
     2359    webframe->setWebCoreFrame(childFrame.get());
    23342360
    23352361    childFrame->tree()->setName(request.frameName());
    … …  
    25702596    // Don't scope if we can't find a frame or a view.
    25712597    // The user may have closed the tab/application, so abort.
    2572     if (!frame() || !frame()->view())
     2598    // Also ignore detached frames, as many find operations report to the main frame.
     2599    if (!frame() || !frame()->view() || !frame()->page())
    25732600        return false;
    25742601
    … …  
    26582685}
    26592686
     2687void WebFrameImpl::willDetachPage()
     2688{
     2689    if (!frame() || !frame()->page())
     2690        return;
     2691
     2692    // Do not expect string scoping results from any frames that got detached
     2693    // in the middle of the operation.
     2694    if (m_scopingInProgress) {
     2695
     2696        // There is a possibility that the frame being detached was the only
     2697        // pending one. We need to make sure final replies can be sent.
     2698        flushCurrentScopingEffort(m_findRequestIdentifier);
     2699
     2700        cancelPendingScopingEffort();
     2701    }
     2702}
     2703
    26602704} // namespace WebKit

  • trunk/Source/WebKit/chromium/src/WebFrameImpl.h

    r129821 r129910  
    3636
    3737#include "Frame.h"
     38#include "FrameDestructionObserver.h"
    3839#include "FrameLoaderClientImpl.h"
    3940#include <wtf/Compiler.h>
    … …  
    7071
    7172// Implementation of WebFrame, note that this is a reference counted object.
    72 class WebFrameImpl : public WebFrame, public RefCounted<WebFrameImpl> {
     73class WebFrameImpl
     74    : public WebFrame
     75    , public RefCounted<WebFrameImpl>
     76    , public WebCore::FrameDestructionObserver {
    7377public:
    7478    // WebFrame methods:
    … …  
    240244    virtual WebString layerTreeAsText(bool showDebugInfo = false) const;
    241245
     246    // WebCore::FrameDestructionObserver methods.
     247    virtual void willDetachPage();
     248
    242249    static PassRefPtr<WebFrameImpl> create(WebFrameClient* client);
    243250    virtual ~WebFrameImpl();
    … …  
    331338    void closing();
    332339
     340    // Sets the local WebCore frame and registers destruction observers.
     341    void setWebCoreFrame(WebCore::Frame*);
     342
    333343    // Notifies the delegate about a new selection rect.
    334344    void reportFindInPageSelection(
    … …  
    382392    bool shouldScopeMatches(const WTF::String& searchText);
    383393
     394    // Removes the current frame from the global scoping effort and triggers any
     395    // updates if appropriate. This method does not mark the scoping operation
     396    // as finished.
     397    void flushCurrentScopingEffort(int identifier);
     398
    384399    // Finishes the current scoping effort and triggers any updates if appropriate.
    385400    void finishCurrentScopingEffort(int identifier);
    … …  
    407422    WebFrameClient* m_client;
    408423
     424    // FIXME: this is redundant as we already have m_frame from FrameDestructionObserver.
    409425    // This is a weak pointer to our corresponding WebCore frame.  A reference to
    410426    // ourselves is held while frame_ is valid.  See our Closing method.
    … …  
    438454    // Keeps track of how many matches this frame has found so far, so that we
    439455    // don't loose count between scoping efforts, and is also used (in conjunction
    440     // with m_lastSearchString and m_scopingComplete) to figure out if we need to
    441     // search the frame again.
     456    // with m_lastSearchString) to figure out if we need to search the frame again.
    442457    int m_lastMatchCount;
    443458
    … …  
    452467    int m_framesScopingCount;
    453468
    454     // Keeps track of whether the scoping effort was completed (the user may
    455     // interrupt it before it completes by submitting a new search).
    456     bool m_scopingComplete;
     469    // Identifier of the latest find-in-page request. Required to be stored in
     470    // the frame in order to reply if required in case the frame is detached.
     471    int m_findRequestIdentifier;
     472
     473    // Keeps track of whether there is an scoping effort ongoing in the frame.
     474    bool m_scopingInProgress;
    457475
    458476    // Keeps track of whether the last find request completed its scoping effort

  • trunk/Source/WebKit/chromium/src/WebNode.cpp

    r129850 r129910  
    224224}
    225225
     226bool WebNode::remove()
     227{
     228    ExceptionCode exceptionCode = 0;
     229    m_private->remove(exceptionCode);
     230    return !exceptionCode;
     231}
     232
    226233bool WebNode::hasNonEmptyBoundingBox() const
    227234{

  • trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp

    r128903 r129910  
    901901    EXPECT_TRUE(mainFrame->find(kFindIdentifier, searchText, options, false, 0));
    902902
     903    mainFrame->resetMatchCount();
     904
    903905    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
    904906        frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
    … …  
    988990    EXPECT_TRUE(mainFrame->findMatchMarkersVersion() != rectsVersion);
    989991
     992    webView->close();
     993}
     994
     995TEST_F(WebFrameTest, FindOnDetachedFrame)
     996{
     997    registerMockedHttpURLLoad("find_in_page.html");
     998    registerMockedHttpURLLoad("find_in_page_frame.html");
     999
     1000    FindUpdateWebFrameClient client;
     1001    WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
     1002    webView->resize(WebSize(640, 480));
     1003    webView->layout();
     1004    webkit_support::RunAllPendingMessages();
     1005
     1006    static const char* kFindString = "result";
     1007    static const int kFindIdentifier = 12345;
     1008
     1009    WebFindOptions options;
     1010    WebString searchText = WebString::fromUTF8(kFindString);
     1011    WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
     1012    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
     1013    RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
     1014
     1015    // Detach the frame before finding.
     1016    EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
     1017
     1018    EXPECT_TRUE(mainFrame->find(kFindIdentifier, searchText, options, false, 0));
     1019    EXPECT_FALSE(secondFrame->find(kFindIdentifier, searchText, options, false, 0));
     1020
     1021    webkit_support::RunAllPendingMessages();
     1022    EXPECT_FALSE(client.findResultsAreReady());
     1023
     1024    mainFrame->resetMatchCount();
     1025
     1026    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
     1027        frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
     1028
     1029    webkit_support::RunAllPendingMessages();
     1030    EXPECT_TRUE(client.findResultsAreReady());
     1031
     1032    holdSecondFrame.release();
     1033    webView->close();
     1034}
     1035
     1036TEST_F(WebFrameTest, FindDetachFrameBeforeScopeStrings)
     1037{
     1038    registerMockedHttpURLLoad("find_in_page.html");
     1039    registerMockedHttpURLLoad("find_in_page_frame.html");
     1040
     1041    FindUpdateWebFrameClient client;
     1042    WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
     1043    webView->resize(WebSize(640, 480));
     1044    webView->layout();
     1045    webkit_support::RunAllPendingMessages();
     1046
     1047    static const char* kFindString = "result";
     1048    static const int kFindIdentifier = 12345;
     1049
     1050    WebFindOptions options;
     1051    WebString searchText = WebString::fromUTF8(kFindString);
     1052    WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
     1053    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
     1054    RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
     1055
     1056    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
     1057        EXPECT_TRUE(frame->find(kFindIdentifier, searchText, options, false, 0));
     1058
     1059    webkit_support::RunAllPendingMessages();
     1060    EXPECT_FALSE(client.findResultsAreReady());
     1061
     1062    // Detach the frame between finding and scoping.
     1063    EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
     1064
     1065    mainFrame->resetMatchCount();
     1066
     1067    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
     1068        frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
     1069
     1070    webkit_support::RunAllPendingMessages();
     1071    EXPECT_TRUE(client.findResultsAreReady());
     1072
     1073    holdSecondFrame.release();
     1074    webView->close();
     1075}
     1076
     1077TEST_F(WebFrameTest, FindDetachFrameWhileScopingStrings)
     1078{
     1079    registerMockedHttpURLLoad("find_in_page.html");
     1080    registerMockedHttpURLLoad("find_in_page_frame.html");
     1081
     1082    FindUpdateWebFrameClient client;
     1083    WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
     1084    webView->resize(WebSize(640, 480));
     1085    webView->layout();
     1086    webkit_support::RunAllPendingMessages();
     1087
     1088    static const char* kFindString = "result";
     1089    static const int kFindIdentifier = 12345;
     1090
     1091    WebFindOptions options;
     1092    WebString searchText = WebString::fromUTF8(kFindString);
     1093    WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
     1094    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
     1095    RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
     1096
     1097    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
     1098        EXPECT_TRUE(frame->find(kFindIdentifier, searchText, options, false, 0));
     1099
     1100    webkit_support::RunAllPendingMessages();
     1101    EXPECT_FALSE(client.findResultsAreReady());
     1102
     1103    mainFrame->resetMatchCount();
     1104
     1105    for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
     1106        frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
     1107
     1108    // The first scopeStringMatches will have reset the state. Detach before it actually scopes.
     1109    EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
     1110
     1111    webkit_support::RunAllPendingMessages();
     1112    EXPECT_TRUE(client.findResultsAreReady());
     1113
     1114    holdSecondFrame.release();
    9901115    webView->close();
    9911116}

  • trunk/Source/WebKit/chromium/tests/data/find_in_page.html

    r126204 r129910  
    77This a find-in-page match rect test.</br>
    88result 00</br>
    9 <iframe src="find_in_page_frame.html" height="300" scrolling="yes"></iframe>
     9<iframe src="find_in_page_frame.html" id="frame" height="300" scrolling="yes"></iframe>
    1010</br>
    1111result 01
Note: See TracChangeset for help on using the changeset viewer.