Skip to content

Commit 51281df

Browse files
author
[email protected]
committed
Race condition with WebKitWebView:is-loading after starting page load
https://bugs.webkit.org/show_bug.cgi?id=136692 Reviewed by Gustavo Noronha Silva. Source/WebKit2: 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: Tools: * 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. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@174497 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 54e6846 commit 51281df

File tree

7 files changed

+209
-54
lines changed

7 files changed

+209
-54
lines changed

‎Source/WebKit2/ChangeLog

+39
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,42 @@
1+
2014-10-08 Carlos Garcia Campos <[email protected]>
2+
3+
Race condition with WebKitWebView:is-loading after starting page load
4+
https://bugs.webkit.org/show_bug.cgi?id=136692
5+
6+
Reviewed by Gustavo Noronha Silva.
7+
8+
Use PageLoadState::Observer to update both is-loading and uri
9+
properties, instead of manually update them. This ensures that our
10+
web view is always in sync with the WebPageProxy so that
11+
webkit_web_view_is_loading() returns true right after requesting
12+
any load. We still need to manually set the is-loading property
13+
only in the case where we delay the emission of the load-changed
14+
signals when waiting for the main resource. The bahaviour is a bit
15+
different but still consistent with what our API documentation says.
16+
17+
* UIProcess/API/gtk/WebKitLoaderClient.cpp:
18+
(attachLoaderClientToView): Remove
19+
didSameDocumentNavigationForFrame implementation, since we are
20+
already notified about the URL change by the PageLoadState::Observer.
21+
(didSameDocumentNavigationForFrame): Deleted.
22+
* UIProcess/API/gtk/WebKitWebView.cpp:
23+
(webkitWebViewSetIsLoading): No longer update the URI when
24+
changing the is-loading property.
25+
(webkitWebViewConstructed): Crate a PageLoadStateObserver and add
26+
it to the PageLoadState.
27+
(webkitWebViewDispose): Remove the PageLoadStateObserver from the PageLoadState.
28+
(webkitWebViewEmitLoadChanged): Add isDelayedEvent parameter to
29+
update the is-loading property accordingly when emitting the
30+
delayed events.
31+
(webkitWebViewEmitDelayedLoadEvents): Pass true as isDelayedEvent
32+
parameter of webkitWebViewEmitLoadChanged().
33+
(webkitWebViewLoadChanged): Pass false as isDelayedEvent parameter
34+
of webkitWebViewEmitLoadChanged().
35+
(webkitWebViewLoadFailed):
36+
(webkitWebViewLoadFailedWithTLSErrors):
37+
(webkitWebViewUpdateURI): Deleted.
38+
* UIProcess/API/gtk/WebKitWebViewPrivate.h:
39+
140
2014-10-08 Christophe Dumez <[email protected]>
241

342
Use is<>() / downcast<>() for RenderBlock objects

‎Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp

+1-9
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,6 @@ static void didFailLoadWithErrorForFrame(WKPageRef, WKFrameRef frame, WKErrorRef
9191
resourceError.failingURL().utf8().data(), webError.get());
9292
}
9393

94-
static void didSameDocumentNavigationForFrame(WKPageRef, WKFrameRef frame, WKSameDocumentNavigationType, WKTypeRef, const void* clientInfo)
95-
{
96-
if (!WKFrameIsMainFrame(frame))
97-
return;
98-
99-
webkitWebViewUpdateURI(WEBKIT_WEB_VIEW(clientInfo));
100-
}
101-
10294
static void didReceiveTitleForFrame(WKPageRef, WKStringRef titleRef, WKFrameRef frameRef, WKTypeRef, const void* clientInfo)
10395
{
10496
if (!WKFrameIsMainFrame(frameRef))
@@ -151,7 +143,7 @@ void attachLoaderClientToView(WebKitWebView* webView)
151143
0, // didFinishDocumentLoadForFrame
152144
didFinishLoadForFrame,
153145
didFailLoadWithErrorForFrame,
154-
didSameDocumentNavigationForFrame,
146+
0, // didSameDocumentNavigationForFrame
155147
didReceiveTitleForFrame,
156148
0, // didFirstLayoutForFrame
157149
0, // didFirstVisuallyNonEmptyLayoutForFrame

‎Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp

+90-36
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ enum {
143143

144144
typedef HashMap<uint64_t, GRefPtr<WebKitWebResource> > LoadingResourcesMap;
145145
typedef HashMap<uint64_t, GRefPtr<GTask> > SnapshotResultsMap;
146+
class PageLoadStateObserver;
146147

147148
struct _WebKitWebViewPrivate {
148149
~_WebKitWebViewPrivate()
@@ -163,6 +164,7 @@ struct _WebKitWebViewPrivate {
163164
CString activeURI;
164165
bool isLoading;
165166

167+
std::unique_ptr<PageLoadStateObserver> loadObserver;
166168
bool waitingForMainResource;
167169
unsigned long mainResourceResponseHandlerID;
168170
WebKitLoadEvent lastDelayedEvent;
@@ -203,6 +205,66 @@ static inline WebPageProxy* getPage(WebKitWebView* webView)
203205
return webkitWebViewBaseGetPage(reinterpret_cast<WebKitWebViewBase*>(webView));
204206
}
205207

208+
static void webkitWebViewSetIsLoading(WebKitWebView* webView, bool isLoading)
209+
{
210+
if (webView->priv->isLoading == isLoading)
211+
return;
212+
213+
webView->priv->isLoading = isLoading;
214+
g_object_notify(G_OBJECT(webView), "is-loading");
215+
}
216+
217+
class PageLoadStateObserver final : public PageLoadState::Observer {
218+
public:
219+
PageLoadStateObserver(WebKitWebView* webView)
220+
: m_webView(webView)
221+
{
222+
}
223+
224+
private:
225+
virtual void willChangeIsLoading() override
226+
{
227+
g_object_freeze_notify(G_OBJECT(m_webView));
228+
}
229+
virtual void didChangeIsLoading() override
230+
{
231+
if (m_webView->priv->waitingForMainResource) {
232+
// The actual load has finished but we haven't emitted the delayed load events yet, so we are still loading.
233+
g_object_thaw_notify(G_OBJECT(m_webView));
234+
return;
235+
}
236+
webkitWebViewSetIsLoading(m_webView, getPage(m_webView)->pageLoadState().isLoading());
237+
g_object_thaw_notify(G_OBJECT(m_webView));
238+
}
239+
240+
virtual void willChangeTitle() override { }
241+
virtual void didChangeTitle() override { }
242+
243+
virtual void willChangeActiveURL() override
244+
{
245+
g_object_freeze_notify(G_OBJECT(m_webView));
246+
}
247+
virtual void didChangeActiveURL() override
248+
{
249+
m_webView->priv->activeURI = getPage(m_webView)->pageLoadState().activeURL().utf8();
250+
g_object_notify(G_OBJECT(m_webView), "uri");
251+
g_object_thaw_notify(G_OBJECT(m_webView));
252+
}
253+
254+
virtual void willChangeHasOnlySecureContent() override { }
255+
virtual void didChangeHasOnlySecureContent() override { }
256+
virtual void willChangeEstimatedProgress() override { }
257+
virtual void didChangeEstimatedProgress() override { }
258+
virtual void willChangeCanGoBack() override { }
259+
virtual void didChangeCanGoBack() override { }
260+
virtual void willChangeCanGoForward() override { }
261+
virtual void didChangeCanGoForward() override { }
262+
virtual void willChangeNetworkRequestsInProgress() override { }
263+
virtual void didChangeNetworkRequestsInProgress() override { }
264+
265+
WebKitWebView* m_webView;
266+
};
267+
206268
static gboolean webkitWebViewLoadFail(WebKitWebView* webView, WebKitLoadEvent, const char* failingURI, GError* error)
207269
{
208270
if (g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)
@@ -493,6 +555,10 @@ static void webkitWebViewConstructed(GObject* object)
493555
if (!priv->settings)
494556
priv->settings = adoptGRef(webkit_settings_new());
495557
webkitWebContextCreatePageForWebView(priv->context, webView, priv->userContentManager.get(), priv->relatedView);
558+
559+
priv->loadObserver = std::make_unique<PageLoadStateObserver>(webView);
560+
getPage(webView)->pageLoadState().addObserver(*priv->loadObserver);
561+
496562
// The related view is only valid during the construction.
497563
priv->relatedView = nullptr;
498564

@@ -591,6 +657,11 @@ static void webkitWebViewDispose(GObject* object)
591657
webkitWebViewDisconnectSettingsSignalHandlers(webView);
592658
webkitWebViewDisconnectFaviconDatabaseSignalHandlers(webView);
593659

660+
if (webView->priv->loadObserver) {
661+
getPage(webView)->pageLoadState().removeObserver(*webView->priv->loadObserver);
662+
webView->priv->loadObserver.reset();
663+
}
664+
594665
webkitWebContextWebViewDestroyed(webView->priv->context, webView);
595666

596667
G_OBJECT_CLASS(webkit_web_view_parent_class)->dispose(object);
@@ -1484,21 +1555,6 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
14841555
WEBKIT_TYPE_AUTHENTICATION_REQUEST);
14851556
}
14861557

1487-
static void webkitWebViewSetIsLoading(WebKitWebView* webView, bool isLoading)
1488-
{
1489-
if (webView->priv->isLoading == isLoading)
1490-
return;
1491-
1492-
webView->priv->isLoading = isLoading;
1493-
g_object_freeze_notify(G_OBJECT(webView));
1494-
g_object_notify(G_OBJECT(webView), "is-loading");
1495-
1496-
// Update the URI if a new load has started.
1497-
if (webView->priv->isLoading)
1498-
webkitWebViewUpdateURI(webView);
1499-
g_object_thaw_notify(G_OBJECT(webView));
1500-
}
1501-
15021558
static void webkitWebViewCancelAuthenticationRequest(WebKitWebView* webView)
15031559
{
15041560
if (!webView->priv->authenticationRequest)
@@ -1508,19 +1564,30 @@ static void webkitWebViewCancelAuthenticationRequest(WebKitWebView* webView)
15081564
webView->priv->authenticationRequest.clear();
15091565
}
15101566

1511-
static void webkitWebViewEmitLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
1567+
static void webkitWebViewEmitLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent, bool isDelayedEvent)
15121568
{
15131569
if (loadEvent == WEBKIT_LOAD_STARTED) {
1514-
webkitWebViewSetIsLoading(webView, true);
15151570
webkitWebViewWatchForChangesInFavicon(webView);
15161571
webkitWebViewCancelAuthenticationRequest(webView);
15171572
} else if (loadEvent == WEBKIT_LOAD_FINISHED) {
1518-
webkitWebViewSetIsLoading(webView, false);
1573+
if (isDelayedEvent) {
1574+
// In case of the delayed event, we need to manually set is-loading to false.
1575+
webkitWebViewSetIsLoading(webView, false);
1576+
}
15191577
webkitWebViewCancelAuthenticationRequest(webView);
15201578
webkitWebViewDisconnectMainResourceResponseChangedSignalHandler(webView);
1521-
} else
1522-
webkitWebViewUpdateURI(webView);
1579+
}
1580+
15231581
g_signal_emit(webView, signals[LOAD_CHANGED], 0, loadEvent);
1582+
1583+
if (isDelayedEvent) {
1584+
if (loadEvent == WEBKIT_LOAD_COMMITTED)
1585+
webView->priv->waitingForMainResource = false;
1586+
else if (loadEvent == WEBKIT_LOAD_FINISHED) {
1587+
// Manually set is-loading again in case a new load was started.
1588+
webkitWebViewSetIsLoading(webView, getPage(webView)->pageLoadState().isLoading());
1589+
}
1590+
}
15241591
}
15251592

15261593
static void webkitWebViewEmitDelayedLoadEvents(WebKitWebView* webView)
@@ -1531,9 +1598,8 @@ static void webkitWebViewEmitDelayedLoadEvents(WebKitWebView* webView)
15311598
ASSERT(priv->lastDelayedEvent == WEBKIT_LOAD_COMMITTED || priv->lastDelayedEvent == WEBKIT_LOAD_FINISHED);
15321599

15331600
if (priv->lastDelayedEvent == WEBKIT_LOAD_FINISHED)
1534-
webkitWebViewEmitLoadChanged(webView, WEBKIT_LOAD_COMMITTED);
1535-
webkitWebViewEmitLoadChanged(webView, priv->lastDelayedEvent);
1536-
priv->waitingForMainResource = false;
1601+
webkitWebViewEmitLoadChanged(webView, WEBKIT_LOAD_COMMITTED, true);
1602+
webkitWebViewEmitLoadChanged(webView, priv->lastDelayedEvent, true);
15371603
}
15381604

15391605
void webkitWebViewLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
@@ -1564,12 +1630,11 @@ void webkitWebViewLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
15641630
if (priv->waitingForMainResource)
15651631
priv->lastDelayedEvent = loadEvent;
15661632
else
1567-
webkitWebViewEmitLoadChanged(webView, loadEvent);
1633+
webkitWebViewEmitLoadChanged(webView, loadEvent, false);
15681634
}
15691635

15701636
void webkitWebViewLoadFailed(WebKitWebView* webView, WebKitLoadEvent loadEvent, const char* failingURI, GError *error)
15711637
{
1572-
webkitWebViewSetIsLoading(webView, false);
15731638
webkitWebViewCancelAuthenticationRequest(webView);
15741639

15751640
gboolean returnValue;
@@ -1579,7 +1644,6 @@ void webkitWebViewLoadFailed(WebKitWebView* webView, WebKitLoadEvent loadEvent,
15791644

15801645
void webkitWebViewLoadFailedWithTLSErrors(WebKitWebView* webView, const char* failingURI, GError* error, GTlsCertificateFlags tlsErrors, GTlsCertificate* certificate)
15811646
{
1582-
webkitWebViewSetIsLoading(webView, false);
15831647
webkitWebViewCancelAuthenticationRequest(webView);
15841648

15851649
WebKitTLSErrorsPolicy tlsErrorsPolicy = webkit_web_context_get_tls_errors_policy(webView->priv->context);
@@ -1612,16 +1676,6 @@ void webkitWebViewSetEstimatedLoadProgress(WebKitWebView* webView, double estima
16121676
g_object_notify(G_OBJECT(webView), "estimated-load-progress");
16131677
}
16141678

1615-
void webkitWebViewUpdateURI(WebKitWebView* webView)
1616-
{
1617-
CString activeURI = getPage(webView)->pageLoadState().activeURL().utf8();
1618-
if (webView->priv->activeURI == activeURI)
1619-
return;
1620-
1621-
webView->priv->activeURI = activeURI;
1622-
g_object_notify(G_OBJECT(webView), "uri");
1623-
}
1624-
16251679
WebPageProxy* webkitWebViewCreateNewPage(WebKitWebView* webView, const WindowFeatures& windowFeatures, WebKitNavigationAction* navigationAction)
16261680
{
16271681
WebKitWebView* newWebView;

‎Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h

-1
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ void webkitWebViewLoadFailed(WebKitWebView*, WebKitLoadEvent, const char* failin
3737
void webkitWebViewLoadFailedWithTLSErrors(WebKitWebView*, const char* failingURI, GError*, GTlsCertificateFlags, GTlsCertificate*);
3838
void webkitWebViewSetEstimatedLoadProgress(WebKitWebView*, double estimatedLoadProgress);
3939
void webkitWebViewSetTitle(WebKitWebView*, const CString&);
40-
void webkitWebViewUpdateURI(WebKitWebView*);
4140
WebKit::WebPageProxy* webkitWebViewCreateNewPage(WebKitWebView*, const WebCore::WindowFeatures&, WebKitNavigationAction*);
4241
void webkitWebViewReadyToShowPage(WebKitWebView*);
4342
void webkitWebViewRunAsModal(WebKitWebView*);

‎Tools/ChangeLog

+24
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,27 @@
1+
2014-10-08 Carlos Garcia Campos <[email protected]>
2+
3+
Race condition with WebKitWebView:is-loading after starting page load
4+
https://bugs.webkit.org/show_bug.cgi?id=136692
5+
6+
Reviewed by Gustavo Noronha Silva.
7+
8+
* TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:
9+
(loadChangedCallback): Only check is-loading is false when load
10+
has finished if the load didn't fail due to a cancellation.
11+
(loadFailedCallback): Only check is-loading is false if the load
12+
didn't fail due to a cancellation.
13+
* TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:
14+
(WebViewTest::loadURI): Check is-loading is true and active URI is
15+
the requested one rigth after requesting the load.
16+
(WebViewTest::loadHtml): Ditto.
17+
(WebViewTest::loadPlainText): Ditto.
18+
(WebViewTest::loadBytes): Ditto.
19+
(WebViewTest::loadRequest): Ditto.
20+
(WebViewTest::loadAlternateHTML): Ditto.
21+
(WebViewTest::goBack): Ditto.
22+
(WebViewTest::goForward): Ditto.
23+
(WebViewTest::goToBackForwardListItem): Ditto.
24+
125
2014-10-08 Brent Fulgham <[email protected]>
226

327
[Win] Resolve various static analyzer warnings in Tools.

‎Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp

+17-6
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,15 @@ static void loadChangedCallback(WebKitWebView* webView, WebKitLoadEvent loadEven
5050
break;
5151
}
5252
case WEBKIT_LOAD_FINISHED:
53-
g_assert(!webkit_web_view_is_loading(webView));
54-
if (!test->m_loadFailed)
53+
if (!test->m_loadFailed) {
54+
g_assert(!webkit_web_view_is_loading(webView));
5555
g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
56+
} else if (!g_error_matches(test->m_error.get(), WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
57+
// When a new load is started before the previous one has finished, we receive the load-finished signal
58+
// of the ongoing load while we already have a provisional URL for the new load. This is the only case
59+
// where isloading is true when the load has finished.
60+
g_assert(!webkit_web_view_is_loading(webView));
61+
}
5662
test->loadFinished();
5763
break;
5864
default:
@@ -63,19 +69,24 @@ static void loadChangedCallback(WebKitWebView* webView, WebKitLoadEvent loadEven
6369
static void loadFailedCallback(WebKitWebView* webView, WebKitLoadEvent loadEvent, const char* failingURI, GError* error, LoadTrackingTest* test)
6470
{
6571
test->m_loadFailed = true;
72+
73+
g_assert(error);
6674
test->m_error.reset(g_error_copy(error));
6775

76+
if (!g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
77+
// When a new load is started before the previous one has finished, we receive the load-failed signal
78+
// of the ongoing load while we already have a provisional URL for the new load. This is the only case
79+
// where is-loading is true when the load has failed.
80+
g_assert(!webkit_web_view_is_loading(webView));
81+
}
82+
6883
switch (loadEvent) {
6984
case WEBKIT_LOAD_STARTED:
70-
g_assert(!webkit_web_view_is_loading(webView));
7185
g_assert_cmpstr(test->m_activeURI.data(), ==, failingURI);
72-
g_assert(error);
7386
test->provisionalLoadFailed(failingURI, error);
7487
break;
7588
case WEBKIT_LOAD_COMMITTED:
76-
g_assert(!webkit_web_view_is_loading(webView));
7789
g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
78-
g_assert(error);
7990
test->loadFailed(failingURI, error);
8091
break;
8192
default:

0 commit comments

Comments
 (0)