Diff
Modified: trunk/Source/WebKit/chromium/ChangeLog (129909 => 129910)
--- trunk/Source/WebKit/chromium/ChangeLog 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/ChangeLog 2012-09-28 16:38:08 UTC (rev 129910)
@@ -1,3 +1,36 @@
+2012-09-28 Leandro Gracia Gil <[email protected]>
+
+ [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:
+
2012-09-28 Kent Tamura <[email protected]>
Clean up Localizer-related functions
Modified: trunk/Source/WebKit/chromium/public/WebNode.h (129909 => 129910)
--- trunk/Source/WebKit/chromium/public/WebNode.h 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/public/WebNode.h 2012-09-28 16:38:08 UTC (rev 129910)
@@ -113,6 +113,7 @@
WEBKIT_EXPORT WebNodeList getElementsByTagName(const WebString&) const;
WEBKIT_EXPORT WebElement rootEditableElement() const;
WEBKIT_EXPORT bool focused() const;
+ WEBKIT_EXPORT bool remove();
// Returns true if the node has a non-empty bounding box in layout.
// This does not 100% guarantee the user can see it, but is pretty close.
Modified: trunk/Source/WebKit/chromium/src/WebFrameImpl.cpp (129909 => 129910)
--- trunk/Source/WebKit/chromium/src/WebFrameImpl.cpp 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/src/WebFrameImpl.cpp 2012-09-28 16:38:08 UTC (rev 129910)
@@ -525,8 +525,6 @@
{
}
-// WebFrame -------------------------------------------------------------------
-
class WebFrameImpl::DeferredScopeStringMatches {
public:
DeferredScopeStringMatches(WebFrameImpl* webFrame,
@@ -559,7 +557,6 @@
bool m_reset;
};
-
// WebFrame -------------------------------------------------------------------
int WebFrame::instanceCount()
@@ -1618,6 +1615,9 @@
bool wrapWithinFrame,
WebRect* selectionRect)
{
+ if (!frame() || !frame()->page())
+ return false;
+
WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
if (!options.findNext)
@@ -1716,13 +1716,15 @@
const WebFindOptions& options,
bool reset)
{
- WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
-
if (reset) {
// This is a brand new search, so we need to reset everything.
// Scoping is just about to begin.
- m_scopingComplete = false;
+ m_scopingInProgress = true;
+ // Need to keep the current identifier locally in order to finish the
+ // request in case the frame is detached during the process.
+ m_findRequestIdentifier = identifier;
+
// Clear highlighting for this frame.
if (frame() && frame()->page() && frame()->editor()->markedTextMatchesAreHighlighted())
frame()->page()->unmarkAllTextMatches();
@@ -1736,7 +1738,9 @@
m_resumeScopingFromRange = 0;
- mainFrameImpl->m_framesScopingCount++;
+ // The view might be null on detached frames.
+ if (frame() && frame()->page())
+ viewImpl()->mainFrameImpl()->m_framesScopingCount++;
// Now, defer scoping until later to allow find operation to finish quickly.
scopeStringMatchesSoon(
@@ -1755,6 +1759,7 @@
return;
}
+ WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
RefPtr<Range> searchRange(rangeOfContents(frame()->document()));
Node* originalEndContainer = searchRange->endContainer();
@@ -1884,21 +1889,30 @@
finishCurrentScopingEffort(identifier);
}
-void WebFrameImpl::finishCurrentScopingEffort(int identifier)
+void WebFrameImpl::flushCurrentScopingEffort(int identifier)
{
+ if (!frame() || !frame()->page())
+ return;
+
WebFrameImpl* mainFrameImpl = viewImpl()->mainFrameImpl();
// This frame has no further scoping left, so it is done. Other frames might,
// of course, continue to scope matches.
- m_scopingComplete = true;
mainFrameImpl->m_framesScopingCount--;
- m_lastFindRequestCompletedWithNoMatches = !m_lastMatchCount;
// If this is the last frame to finish scoping we need to trigger the final
// update to be sent.
if (!mainFrameImpl->m_framesScopingCount)
mainFrameImpl->increaseMatchCount(0, identifier);
+}
+void WebFrameImpl::finishCurrentScopingEffort(int identifier)
+{
+ flushCurrentScopingEffort(identifier);
+
+ m_scopingInProgress = false;
+ m_lastFindRequestCompletedWithNoMatches = !m_lastMatchCount;
+
// This frame is done, so show any scrollbar tickmarks we haven't drawn yet.
invalidateArea(InvalidateScrollbar);
}
@@ -1910,8 +1924,11 @@
m_activeMatchIndexInCurrentFrame = -1;
- if (!m_scopingComplete)
+ // Last request didn't complete.
+ if (m_scopingInProgress)
m_lastFindRequestCompletedWithNoMatches = false;
+
+ m_scopingInProgress = false;
}
void WebFrameImpl::increaseMatchCount(int count, int identifier)
@@ -2273,7 +2290,8 @@
}
WebFrameImpl::WebFrameImpl(WebFrameClient* client)
- : m_frameLoaderClient(this)
+ : FrameDestructionObserver(0)
+ , m_frameLoaderClient(this)
, m_client(client)
, m_frame(0)
, m_currentActiveMatchFrame(0)
@@ -2283,7 +2301,8 @@
, m_lastMatchCount(-1)
, m_totalMatchCount(-1)
, m_framesScopingCount(-1)
- , m_scopingComplete(false)
+ , m_findRequestIdentifier(-1)
+ , m_scopingInProgress(false)
, m_lastFindRequestCompletedWithNoMatches(false)
, m_nextInvalidateAfter(0)
, m_findMatchMarkersVersion(0)
@@ -2304,10 +2323,17 @@
cancelPendingScopingEffort();
}
+void WebFrameImpl::setWebCoreFrame(WebCore::Frame* frame)
+{
+ ASSERT(frame);
+ m_frame = frame;
+ observeFrame(m_frame);
+}
+
void WebFrameImpl::initializeAsMainFrame(WebCore::Page* page)
{
RefPtr<Frame> frame = Frame::create(page, 0, &m_frameLoaderClient);
- m_frame = frame.get();
+ setWebCoreFrame(frame.get());
// Add reference on behalf of FrameLoader. See comments in
// WebFrameLoaderClient::frameLoaderDestroyed for more info.
@@ -2330,7 +2356,7 @@
RefPtr<Frame> childFrame = Frame::create(
m_frame->page(), ownerElement, &webframe->m_frameLoaderClient);
- webframe->m_frame = childFrame.get();
+ webframe->setWebCoreFrame(childFrame.get());
childFrame->tree()->setName(request.frameName());
@@ -2569,7 +2595,8 @@
{
// Don't scope if we can't find a frame or a view.
// The user may have closed the tab/application, so abort.
- if (!frame() || !frame()->view())
+ // Also ignore detached frames, as many find operations report to the main frame.
+ if (!frame() || !frame()->view() || !frame()->page())
return false;
ASSERT(frame()->document() && frame()->view());
@@ -2657,4 +2684,21 @@
m_frame->document()->loader()->writer()->replaceDocument(scriptResult, ownerDocument.get());
}
+void WebFrameImpl::willDetachPage()
+{
+ if (!frame() || !frame()->page())
+ return;
+
+ // Do not expect string scoping results from any frames that got detached
+ // in the middle of the operation.
+ if (m_scopingInProgress) {
+
+ // There is a possibility that the frame being detached was the only
+ // pending one. We need to make sure final replies can be sent.
+ flushCurrentScopingEffort(m_findRequestIdentifier);
+
+ cancelPendingScopingEffort();
+ }
+}
+
} // namespace WebKit
Modified: trunk/Source/WebKit/chromium/src/WebFrameImpl.h (129909 => 129910)
--- trunk/Source/WebKit/chromium/src/WebFrameImpl.h 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/src/WebFrameImpl.h 2012-09-28 16:38:08 UTC (rev 129910)
@@ -35,6 +35,7 @@
#include "WebFrame.h"
#include "Frame.h"
+#include "FrameDestructionObserver.h"
#include "FrameLoaderClientImpl.h"
#include <wtf/Compiler.h>
#include <wtf/OwnPtr.h>
@@ -69,7 +70,10 @@
template <typename T> class WebVector;
// Implementation of WebFrame, note that this is a reference counted object.
-class WebFrameImpl : public WebFrame, public RefCounted<WebFrameImpl> {
+class WebFrameImpl
+ : public WebFrame
+ , public RefCounted<WebFrameImpl>
+ , public WebCore::FrameDestructionObserver {
public:
// WebFrame methods:
virtual WebString uniqueName() const;
@@ -239,6 +243,9 @@
virtual bool selectionStartHasSpellingMarkerFor(int from, int length) const;
virtual WebString layerTreeAsText(bool showDebugInfo = false) const;
+ // WebCore::FrameDestructionObserver methods.
+ virtual void willDetachPage();
+
static PassRefPtr<WebFrameImpl> create(WebFrameClient* client);
virtual ~WebFrameImpl();
@@ -330,6 +337,9 @@
// WebFrameLoaderClient
void closing();
+ // Sets the local WebCore frame and registers destruction observers.
+ void setWebCoreFrame(WebCore::Frame*);
+
// Notifies the delegate about a new selection rect.
void reportFindInPageSelection(
const WebRect& selectionRect, int activeMatchOrdinal, int identifier);
@@ -381,6 +391,11 @@
// was searched.
bool shouldScopeMatches(const WTF::String& searchText);
+ // Removes the current frame from the global scoping effort and triggers any
+ // updates if appropriate. This method does not mark the scoping operation
+ // as finished.
+ void flushCurrentScopingEffort(int identifier);
+
// Finishes the current scoping effort and triggers any updates if appropriate.
void finishCurrentScopingEffort(int identifier);
@@ -406,6 +421,7 @@
WebFrameClient* m_client;
+ // FIXME: this is redundant as we already have m_frame from FrameDestructionObserver.
// This is a weak pointer to our corresponding WebCore frame. A reference to
// ourselves is held while frame_ is valid. See our Closing method.
WebCore::Frame* m_frame;
@@ -437,8 +453,7 @@
// Keeps track of how many matches this frame has found so far, so that we
// don't loose count between scoping efforts, and is also used (in conjunction
- // with m_lastSearchString and m_scopingComplete) to figure out if we need to
- // search the frame again.
+ // with m_lastSearchString) to figure out if we need to search the frame again.
int m_lastMatchCount;
// This variable keeps a cumulative total of matches found so far for ALL the
@@ -451,10 +466,13 @@
// It should be -1 for all other frames.
int m_framesScopingCount;
- // Keeps track of whether the scoping effort was completed (the user may
- // interrupt it before it completes by submitting a new search).
- bool m_scopingComplete;
+ // Identifier of the latest find-in-page request. Required to be stored in
+ // the frame in order to reply if required in case the frame is detached.
+ int m_findRequestIdentifier;
+ // Keeps track of whether there is an scoping effort ongoing in the frame.
+ bool m_scopingInProgress;
+
// Keeps track of whether the last find request completed its scoping effort
// without finding any matches in this frame.
bool m_lastFindRequestCompletedWithNoMatches;
Modified: trunk/Source/WebKit/chromium/src/WebNode.cpp (129909 => 129910)
--- trunk/Source/WebKit/chromium/src/WebNode.cpp 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/src/WebNode.cpp 2012-09-28 16:38:08 UTC (rev 129910)
@@ -223,6 +223,13 @@
return m_private->focused();
}
+bool WebNode::remove()
+{
+ ExceptionCode exceptionCode = 0;
+ m_private->remove(exceptionCode);
+ return !exceptionCode;
+}
+
bool WebNode::hasNonEmptyBoundingBox() const
{
m_private->document()->updateLayoutIgnorePendingStylesheets();
Modified: trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp (129909 => 129910)
--- trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp 2012-09-28 16:38:08 UTC (rev 129910)
@@ -900,6 +900,8 @@
WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
EXPECT_TRUE(mainFrame->find(kFindIdentifier, searchText, options, false, 0));
+ mainFrame->resetMatchCount();
+
for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
@@ -990,6 +992,129 @@
webView->close();
}
+TEST_F(WebFrameTest, FindOnDetachedFrame)
+{
+ registerMockedHttpURLLoad("find_in_page.html");
+ registerMockedHttpURLLoad("find_in_page_frame.html");
+
+ FindUpdateWebFrameClient client;
+ WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
+ webView->resize(WebSize(640, 480));
+ webView->layout();
+ webkit_support::RunAllPendingMessages();
+
+ static const char* kFindString = "result";
+ static const int kFindIdentifier = 12345;
+
+ WebFindOptions options;
+ WebString searchText = WebString::fromUTF8(kFindString);
+ WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
+ WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
+ RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
+
+ // Detach the frame before finding.
+ EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
+
+ EXPECT_TRUE(mainFrame->find(kFindIdentifier, searchText, options, false, 0));
+ EXPECT_FALSE(secondFrame->find(kFindIdentifier, searchText, options, false, 0));
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_FALSE(client.findResultsAreReady());
+
+ mainFrame->resetMatchCount();
+
+ for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
+ frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_TRUE(client.findResultsAreReady());
+
+ holdSecondFrame.release();
+ webView->close();
+}
+
+TEST_F(WebFrameTest, FindDetachFrameBeforeScopeStrings)
+{
+ registerMockedHttpURLLoad("find_in_page.html");
+ registerMockedHttpURLLoad("find_in_page_frame.html");
+
+ FindUpdateWebFrameClient client;
+ WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
+ webView->resize(WebSize(640, 480));
+ webView->layout();
+ webkit_support::RunAllPendingMessages();
+
+ static const char* kFindString = "result";
+ static const int kFindIdentifier = 12345;
+
+ WebFindOptions options;
+ WebString searchText = WebString::fromUTF8(kFindString);
+ WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
+ WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
+ RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
+
+ for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
+ EXPECT_TRUE(frame->find(kFindIdentifier, searchText, options, false, 0));
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_FALSE(client.findResultsAreReady());
+
+ // Detach the frame between finding and scoping.
+ EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
+
+ mainFrame->resetMatchCount();
+
+ for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
+ frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_TRUE(client.findResultsAreReady());
+
+ holdSecondFrame.release();
+ webView->close();
+}
+
+TEST_F(WebFrameTest, FindDetachFrameWhileScopingStrings)
+{
+ registerMockedHttpURLLoad("find_in_page.html");
+ registerMockedHttpURLLoad("find_in_page_frame.html");
+
+ FindUpdateWebFrameClient client;
+ WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "find_in_page.html", true, &client);
+ webView->resize(WebSize(640, 480));
+ webView->layout();
+ webkit_support::RunAllPendingMessages();
+
+ static const char* kFindString = "result";
+ static const int kFindIdentifier = 12345;
+
+ WebFindOptions options;
+ WebString searchText = WebString::fromUTF8(kFindString);
+ WebFrameImpl* mainFrame = static_cast<WebFrameImpl*>(webView->mainFrame());
+ WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
+ RefPtr<WebCore::Frame> holdSecondFrame = secondFrame->frame();
+
+ for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
+ EXPECT_TRUE(frame->find(kFindIdentifier, searchText, options, false, 0));
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_FALSE(client.findResultsAreReady());
+
+ mainFrame->resetMatchCount();
+
+ for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false))
+ frame->scopeStringMatches(kFindIdentifier, searchText, options, true);
+
+ // The first scopeStringMatches will have reset the state. Detach before it actually scopes.
+ EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());
+
+ webkit_support::RunAllPendingMessages();
+ EXPECT_TRUE(client.findResultsAreReady());
+
+ holdSecondFrame.release();
+ webView->close();
+}
+
static WebView* createWebViewForTextSelection(const std::string& url)
{
WebView* webView = FrameTestHelpers::createWebViewAndLoad(url, true);
Modified: trunk/Source/WebKit/chromium/tests/data/find_in_page.html (129909 => 129910)
--- trunk/Source/WebKit/chromium/tests/data/find_in_page.html 2012-09-28 16:34:07 UTC (rev 129909)
+++ trunk/Source/WebKit/chromium/tests/data/find_in_page.html 2012-09-28 16:38:08 UTC (rev 129910)
@@ -6,7 +6,7 @@
<body>
This a find-in-page match rect test.</br>
result 00</br>
-<iframe src="" height="300" scrolling="yes"></iframe>
+<iframe src="" id="frame" height="300" scrolling="yes"></iframe>
</br>
result 01
</body>
