Diff
Modified: trunk/Source/WebKit2/ChangeLog (174496 => 174497)
--- trunk/Source/WebKit2/ChangeLog 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Source/WebKit2/ChangeLog 2014-10-09 06:33:46 UTC (rev 174497)
@@ -1,3 +1,42 @@
+2014-10-08 Carlos Garcia Campos <[email protected]>
+
+ Race condition with WebKitWebView:is-loading after starting page load
+ https://bugs.webkit.org/show_bug.cgi?id=136692
+
+ Reviewed by Gustavo Noronha Silva.
+
+ Use PageLoadState::Observer to update both is-loading and uri
+ properties, instead of manually update them. This ensures that our
+ web view is always in sync with the WebPageProxy so that
+ webkit_web_view_is_loading() returns true right after requesting
+ any load. We still need to manually set the is-loading property
+ only in the case where we delay the emission of the load-changed
+ signals when waiting for the main resource. The bahaviour is a bit
+ different but still consistent with what our API documentation says.
+
+ * UIProcess/API/gtk/WebKitLoaderClient.cpp:
+ (attachLoaderClientToView): Remove
+ didSameDocumentNavigationForFrame implementation, since we are
+ already notified about the URL change by the PageLoadState::Observer.
+ (didSameDocumentNavigationForFrame): Deleted.
+ * UIProcess/API/gtk/WebKitWebView.cpp:
+ (webkitWebViewSetIsLoading): No longer update the URI when
+ changing the is-loading property.
+ (webkitWebViewConstructed): Crate a PageLoadStateObserver and add
+ it to the PageLoadState.
+ (webkitWebViewDispose): Remove the PageLoadStateObserver from the PageLoadState.
+ (webkitWebViewEmitLoadChanged): Add isDelayedEvent parameter to
+ update the is-loading property accordingly when emitting the
+ delayed events.
+ (webkitWebViewEmitDelayedLoadEvents): Pass true as isDelayedEvent
+ parameter of webkitWebViewEmitLoadChanged().
+ (webkitWebViewLoadChanged): Pass false as isDelayedEvent parameter
+ of webkitWebViewEmitLoadChanged().
+ (webkitWebViewLoadFailed):
+ (webkitWebViewLoadFailedWithTLSErrors):
+ (webkitWebViewUpdateURI): Deleted.
+ * UIProcess/API/gtk/WebKitWebViewPrivate.h:
+
2014-10-08 Christophe Dumez <[email protected]>
Use is<>() / downcast<>() for RenderBlock objects
Modified: trunk/Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp (174496 => 174497)
--- trunk/Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp 2014-10-09 06:33:46 UTC (rev 174497)
@@ -91,14 +91,6 @@
resourceError.failingURL().utf8().data(), webError.get());
}
-static void didSameDocumentNavigationForFrame(WKPageRef, WKFrameRef frame, WKSameDocumentNavigationType, WKTypeRef, const void* clientInfo)
-{
- if (!WKFrameIsMainFrame(frame))
- return;
-
- webkitWebViewUpdateURI(WEBKIT_WEB_VIEW(clientInfo));
-}
-
static void didReceiveTitleForFrame(WKPageRef, WKStringRef titleRef, WKFrameRef frameRef, WKTypeRef, const void* clientInfo)
{
if (!WKFrameIsMainFrame(frameRef))
@@ -151,7 +143,7 @@
0, // didFinishDocumentLoadForFrame
didFinishLoadForFrame,
didFailLoadWithErrorForFrame,
- didSameDocumentNavigationForFrame,
+ 0, // didSameDocumentNavigationForFrame
didReceiveTitleForFrame,
0, // didFirstLayoutForFrame
0, // didFirstVisuallyNonEmptyLayoutForFrame
Modified: trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp (174496 => 174497)
--- trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp 2014-10-09 06:33:46 UTC (rev 174497)
@@ -143,6 +143,7 @@
typedef HashMap<uint64_t, GRefPtr<WebKitWebResource> > LoadingResourcesMap;
typedef HashMap<uint64_t, GRefPtr<GTask> > SnapshotResultsMap;
+class PageLoadStateObserver;
struct _WebKitWebViewPrivate {
~_WebKitWebViewPrivate()
@@ -163,6 +164,7 @@
CString activeURI;
bool isLoading;
+ std::unique_ptr<PageLoadStateObserver> loadObserver;
bool waitingForMainResource;
unsigned long mainResourceResponseHandlerID;
WebKitLoadEvent lastDelayedEvent;
@@ -203,6 +205,66 @@
return webkitWebViewBaseGetPage(reinterpret_cast<WebKitWebViewBase*>(webView));
}
+static void webkitWebViewSetIsLoading(WebKitWebView* webView, bool isLoading)
+{
+ if (webView->priv->isLoading == isLoading)
+ return;
+
+ webView->priv->isLoading = isLoading;
+ g_object_notify(G_OBJECT(webView), "is-loading");
+}
+
+class PageLoadStateObserver final : public PageLoadState::Observer {
+public:
+ PageLoadStateObserver(WebKitWebView* webView)
+ : m_webView(webView)
+ {
+ }
+
+private:
+ virtual void willChangeIsLoading() override
+ {
+ g_object_freeze_notify(G_OBJECT(m_webView));
+ }
+ virtual void didChangeIsLoading() override
+ {
+ if (m_webView->priv->waitingForMainResource) {
+ // The actual load has finished but we haven't emitted the delayed load events yet, so we are still loading.
+ g_object_thaw_notify(G_OBJECT(m_webView));
+ return;
+ }
+ webkitWebViewSetIsLoading(m_webView, getPage(m_webView)->pageLoadState().isLoading());
+ g_object_thaw_notify(G_OBJECT(m_webView));
+ }
+
+ virtual void willChangeTitle() override { }
+ virtual void didChangeTitle() override { }
+
+ virtual void willChangeActiveURL() override
+ {
+ g_object_freeze_notify(G_OBJECT(m_webView));
+ }
+ virtual void didChangeActiveURL() override
+ {
+ m_webView->priv->activeURI = getPage(m_webView)->pageLoadState().activeURL().utf8();
+ g_object_notify(G_OBJECT(m_webView), "uri");
+ g_object_thaw_notify(G_OBJECT(m_webView));
+ }
+
+ virtual void willChangeHasOnlySecureContent() override { }
+ virtual void didChangeHasOnlySecureContent() override { }
+ virtual void willChangeEstimatedProgress() override { }
+ virtual void didChangeEstimatedProgress() override { }
+ virtual void willChangeCanGoBack() override { }
+ virtual void didChangeCanGoBack() override { }
+ virtual void willChangeCanGoForward() override { }
+ virtual void didChangeCanGoForward() override { }
+ virtual void willChangeNetworkRequestsInProgress() override { }
+ virtual void didChangeNetworkRequestsInProgress() override { }
+
+ WebKitWebView* m_webView;
+};
+
static gboolean webkitWebViewLoadFail(WebKitWebView* webView, WebKitLoadEvent, const char* failingURI, GError* error)
{
if (g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)
@@ -493,6 +555,10 @@
if (!priv->settings)
priv->settings = adoptGRef(webkit_settings_new());
webkitWebContextCreatePageForWebView(priv->context, webView, priv->userContentManager.get(), priv->relatedView);
+
+ priv->loadObserver = std::make_unique<PageLoadStateObserver>(webView);
+ getPage(webView)->pageLoadState().addObserver(*priv->loadObserver);
+
// The related view is only valid during the construction.
priv->relatedView = nullptr;
@@ -591,6 +657,11 @@
webkitWebViewDisconnectSettingsSignalHandlers(webView);
webkitWebViewDisconnectFaviconDatabaseSignalHandlers(webView);
+ if (webView->priv->loadObserver) {
+ getPage(webView)->pageLoadState().removeObserver(*webView->priv->loadObserver);
+ webView->priv->loadObserver.reset();
+ }
+
webkitWebContextWebViewDestroyed(webView->priv->context, webView);
G_OBJECT_CLASS(webkit_web_view_parent_class)->dispose(object);
@@ -1484,21 +1555,6 @@
WEBKIT_TYPE_AUTHENTICATION_REQUEST);
}
-static void webkitWebViewSetIsLoading(WebKitWebView* webView, bool isLoading)
-{
- if (webView->priv->isLoading == isLoading)
- return;
-
- webView->priv->isLoading = isLoading;
- g_object_freeze_notify(G_OBJECT(webView));
- g_object_notify(G_OBJECT(webView), "is-loading");
-
- // Update the URI if a new load has started.
- if (webView->priv->isLoading)
- webkitWebViewUpdateURI(webView);
- g_object_thaw_notify(G_OBJECT(webView));
-}
-
static void webkitWebViewCancelAuthenticationRequest(WebKitWebView* webView)
{
if (!webView->priv->authenticationRequest)
@@ -1508,19 +1564,30 @@
webView->priv->authenticationRequest.clear();
}
-static void webkitWebViewEmitLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
+static void webkitWebViewEmitLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent, bool isDelayedEvent)
{
if (loadEvent == WEBKIT_LOAD_STARTED) {
- webkitWebViewSetIsLoading(webView, true);
webkitWebViewWatchForChangesInFavicon(webView);
webkitWebViewCancelAuthenticationRequest(webView);
} else if (loadEvent == WEBKIT_LOAD_FINISHED) {
- webkitWebViewSetIsLoading(webView, false);
+ if (isDelayedEvent) {
+ // In case of the delayed event, we need to manually set is-loading to false.
+ webkitWebViewSetIsLoading(webView, false);
+ }
webkitWebViewCancelAuthenticationRequest(webView);
webkitWebViewDisconnectMainResourceResponseChangedSignalHandler(webView);
- } else
- webkitWebViewUpdateURI(webView);
+ }
+
g_signal_emit(webView, signals[LOAD_CHANGED], 0, loadEvent);
+
+ if (isDelayedEvent) {
+ if (loadEvent == WEBKIT_LOAD_COMMITTED)
+ webView->priv->waitingForMainResource = false;
+ else if (loadEvent == WEBKIT_LOAD_FINISHED) {
+ // Manually set is-loading again in case a new load was started.
+ webkitWebViewSetIsLoading(webView, getPage(webView)->pageLoadState().isLoading());
+ }
+ }
}
static void webkitWebViewEmitDelayedLoadEvents(WebKitWebView* webView)
@@ -1531,9 +1598,8 @@
ASSERT(priv->lastDelayedEvent == WEBKIT_LOAD_COMMITTED || priv->lastDelayedEvent == WEBKIT_LOAD_FINISHED);
if (priv->lastDelayedEvent == WEBKIT_LOAD_FINISHED)
- webkitWebViewEmitLoadChanged(webView, WEBKIT_LOAD_COMMITTED);
- webkitWebViewEmitLoadChanged(webView, priv->lastDelayedEvent);
- priv->waitingForMainResource = false;
+ webkitWebViewEmitLoadChanged(webView, WEBKIT_LOAD_COMMITTED, true);
+ webkitWebViewEmitLoadChanged(webView, priv->lastDelayedEvent, true);
}
void webkitWebViewLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
@@ -1564,12 +1630,11 @@
if (priv->waitingForMainResource)
priv->lastDelayedEvent = loadEvent;
else
- webkitWebViewEmitLoadChanged(webView, loadEvent);
+ webkitWebViewEmitLoadChanged(webView, loadEvent, false);
}
void webkitWebViewLoadFailed(WebKitWebView* webView, WebKitLoadEvent loadEvent, const char* failingURI, GError *error)
{
- webkitWebViewSetIsLoading(webView, false);
webkitWebViewCancelAuthenticationRequest(webView);
gboolean returnValue;
@@ -1579,7 +1644,6 @@
void webkitWebViewLoadFailedWithTLSErrors(WebKitWebView* webView, const char* failingURI, GError* error, GTlsCertificateFlags tlsErrors, GTlsCertificate* certificate)
{
- webkitWebViewSetIsLoading(webView, false);
webkitWebViewCancelAuthenticationRequest(webView);
WebKitTLSErrorsPolicy tlsErrorsPolicy = webkit_web_context_get_tls_errors_policy(webView->priv->context);
@@ -1612,16 +1676,6 @@
g_object_notify(G_OBJECT(webView), "estimated-load-progress");
}
-void webkitWebViewUpdateURI(WebKitWebView* webView)
-{
- CString activeURI = getPage(webView)->pageLoadState().activeURL().utf8();
- if (webView->priv->activeURI == activeURI)
- return;
-
- webView->priv->activeURI = activeURI;
- g_object_notify(G_OBJECT(webView), "uri");
-}
-
WebPageProxy* webkitWebViewCreateNewPage(WebKitWebView* webView, const WindowFeatures& windowFeatures, WebKitNavigationAction* navigationAction)
{
WebKitWebView* newWebView;
Modified: trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h (174496 => 174497)
--- trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h 2014-10-09 06:33:46 UTC (rev 174497)
@@ -37,7 +37,6 @@
void webkitWebViewLoadFailedWithTLSErrors(WebKitWebView*, const char* failingURI, GError*, GTlsCertificateFlags, GTlsCertificate*);
void webkitWebViewSetEstimatedLoadProgress(WebKitWebView*, double estimatedLoadProgress);
void webkitWebViewSetTitle(WebKitWebView*, const CString&);
-void webkitWebViewUpdateURI(WebKitWebView*);
WebKit::WebPageProxy* webkitWebViewCreateNewPage(WebKitWebView*, const WebCore::WindowFeatures&, WebKitNavigationAction*);
void webkitWebViewReadyToShowPage(WebKitWebView*);
void webkitWebViewRunAsModal(WebKitWebView*);
Modified: trunk/Tools/ChangeLog (174496 => 174497)
--- trunk/Tools/ChangeLog 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Tools/ChangeLog 2014-10-09 06:33:46 UTC (rev 174497)
@@ -1,3 +1,27 @@
+2014-10-08 Carlos Garcia Campos <[email protected]>
+
+ Race condition with WebKitWebView:is-loading after starting page load
+ https://bugs.webkit.org/show_bug.cgi?id=136692
+
+ Reviewed by Gustavo Noronha Silva.
+
+ * TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:
+ (loadChangedCallback): Only check is-loading is false when load
+ has finished if the load didn't fail due to a cancellation.
+ (loadFailedCallback): Only check is-loading is false if the load
+ didn't fail due to a cancellation.
+ * TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:
+ (WebViewTest::loadURI): Check is-loading is true and active URI is
+ the requested one rigth after requesting the load.
+ (WebViewTest::loadHtml): Ditto.
+ (WebViewTest::loadPlainText): Ditto.
+ (WebViewTest::loadBytes): Ditto.
+ (WebViewTest::loadRequest): Ditto.
+ (WebViewTest::loadAlternateHTML): Ditto.
+ (WebViewTest::goBack): Ditto.
+ (WebViewTest::goForward): Ditto.
+ (WebViewTest::goToBackForwardListItem): Ditto.
+
2014-10-08 Brent Fulgham <[email protected]>
[Win] Resolve various static analyzer warnings in Tools.
Modified: trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp (174496 => 174497)
--- trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp 2014-10-09 06:33:46 UTC (rev 174497)
@@ -50,9 +50,15 @@
break;
}
case WEBKIT_LOAD_FINISHED:
- g_assert(!webkit_web_view_is_loading(webView));
- if (!test->m_loadFailed)
+ if (!test->m_loadFailed) {
+ g_assert(!webkit_web_view_is_loading(webView));
g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
+ } else if (!g_error_matches(test->m_error.get(), WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
+ // When a new load is started before the previous one has finished, we receive the load-finished signal
+ // of the ongoing load while we already have a provisional URL for the new load. This is the only case
+ // where isloading is true when the load has finished.
+ g_assert(!webkit_web_view_is_loading(webView));
+ }
test->loadFinished();
break;
default:
@@ -63,19 +69,24 @@
static void loadFailedCallback(WebKitWebView* webView, WebKitLoadEvent loadEvent, const char* failingURI, GError* error, LoadTrackingTest* test)
{
test->m_loadFailed = true;
+
+ g_assert(error);
test->m_error.reset(g_error_copy(error));
+ if (!g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
+ // When a new load is started before the previous one has finished, we receive the load-failed signal
+ // of the ongoing load while we already have a provisional URL for the new load. This is the only case
+ // where is-loading is true when the load has failed.
+ g_assert(!webkit_web_view_is_loading(webView));
+ }
+
switch (loadEvent) {
case WEBKIT_LOAD_STARTED:
- g_assert(!webkit_web_view_is_loading(webView));
g_assert_cmpstr(test->m_activeURI.data(), ==, failingURI);
- g_assert(error);
test->provisionalLoadFailed(failingURI, error);
break;
case WEBKIT_LOAD_COMMITTED:
- g_assert(!webkit_web_view_is_loading(webView));
g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
- g_assert(error);
test->loadFailed(failingURI, error);
break;
default:
Modified: trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp (174496 => 174497)
--- trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp 2014-10-09 06:26:07 UTC (rev 174496)
+++ trunk/Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp 2014-10-09 06:33:46 UTC (rev 174497)
@@ -54,6 +54,8 @@
{
m_activeURI = uri;
webkit_web_view_load_uri(m_webView, uri);
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
}
void WebViewTest::loadHtml(const char* html, const char* baseURI)
@@ -63,12 +65,20 @@
else
m_activeURI = baseURI;
webkit_web_view_load_html(m_webView, html, baseURI);
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
}
void WebViewTest::loadPlainText(const char* plainText)
{
m_activeURI = "about:blank";
webkit_web_view_load_plain_text(m_webView, plainText);
+#if 0
+ // FIXME: Pending API request URL no set when loading plain text.
+ // See https://bugs.webkit.org/show_bug.cgi?id=136916.
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
+#endif
}
void WebViewTest::loadBytes(GBytes* bytes, const char* mimeType, const char* encoding, const char* baseURI)
@@ -78,23 +88,38 @@
else
m_activeURI = baseURI;
webkit_web_view_load_bytes(m_webView, bytes, mimeType, encoding, baseURI);
+#if 0
+ // FIXME: Pending API request URL no set when loading data.
+ // See https://bugs.webkit.org/show_bug.cgi?id=136916.
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
+#endif
}
void WebViewTest::loadRequest(WebKitURIRequest* request)
{
m_activeURI = webkit_uri_request_get_uri(request);
webkit_web_view_load_request(m_webView, request);
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
}
void WebViewTest::loadAlternateHTML(const char* html, const char* contentURI, const char* baseURI)
{
m_activeURI = contentURI;
webkit_web_view_load_alternate_html(m_webView, html, contentURI, baseURI);
+#if 0
+ // FIXME: Pending API request URL no set when loading Alternate HTML.
+ // See https://bugs.webkit.org/show_bug.cgi?id=136916.
+ g_assert(webkit_web_view_is_loading(m_webView));
+#endif
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
}
void WebViewTest::goBack()
{
- if (webkit_web_view_can_go_back(m_webView)) {
+ bool canGoBack = webkit_web_view_can_go_back(m_webView);
+ if (canGoBack) {
WebKitBackForwardList* list = webkit_web_view_get_back_forward_list(m_webView);
WebKitBackForwardListItem* item = webkit_back_forward_list_get_nth_item(list, -1);
m_activeURI = webkit_back_forward_list_item_get_original_uri(item);
@@ -102,11 +127,16 @@
// Call go_back even when can_go_back returns FALSE to check nothing happens.
webkit_web_view_go_back(m_webView);
+ if (canGoBack) {
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
+ }
}
void WebViewTest::goForward()
{
- if (webkit_web_view_can_go_forward(m_webView)) {
+ bool canGoForward = webkit_web_view_can_go_forward(m_webView);
+ if (canGoForward) {
WebKitBackForwardList* list = webkit_web_view_get_back_forward_list(m_webView);
WebKitBackForwardListItem* item = webkit_back_forward_list_get_nth_item(list, 1);
m_activeURI = webkit_back_forward_list_item_get_original_uri(item);
@@ -114,12 +144,18 @@
// Call go_forward even when can_go_forward returns FALSE to check nothing happens.
webkit_web_view_go_forward(m_webView);
+ if (canGoForward) {
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
+ }
}
void WebViewTest::goToBackForwardListItem(WebKitBackForwardListItem* item)
{
m_activeURI = webkit_back_forward_list_item_get_original_uri(item);
webkit_web_view_go_to_back_forward_list_item(m_webView, item);
+ g_assert(webkit_web_view_is_loading(m_webView));
+ g_assert_cmpstr(webkit_web_view_get_uri(m_webView), ==, m_activeURI.data());
}
void WebViewTest::quitMainLoop()
