Bug 242248 – [WPE][GTK] 'Stop' button does not STOP

• Status: REOPENED
• Component: WebKitGTK

I press the "STOP" button in my browser, and the page ignores my command and keeps downloading stuff. Apparently "STOP" has a different meaning to WebKit and Chromium than to the rest of the human species.

This is a major bug that not only is an extreme annoyance, but severely affects usability of WebKit on slow connections like mine. It's infuriating when I command the browser to "F__KING STOP LOADING, RIGHT NOW" and it just keeps going merrily on its way, sucking up my entire 10k/sec downloading some page resource that I have absolutely no use for. At the moment I have to direct Chromium to use a proxy server so that I can kill the proxy when I really, seriously want it to STOP loading stuff. Otherwise my web browsing session will grind to a halt as the browser just keeps on downloading random stuff. I can't even do that with WebKit any more, since WebKit SSL over my Squid proxy no longer works on the latest WebKit version for some inexplicable reason. (See the bug I filed about that also.)

[Michael Catanzaro]

I believe webkit_web_view_stop_loading() has been broken for several years, perhaps even more.

It's been broken since before the Blink/WebKit split, and that was in what, 2012? 

I wish I could express genuine shock and surprise that this could possibly be, but sadly, that's impossible for me anymore. I've seen far too much outrageously broken code (with WONTFIX applied) to have any faith left in humanity anymore. How humans even managed to invent computers in the first place, or any technology at all, is a mystery to me at this point. 

It has become clear that the only humans who matter at all to the Silicon Valley programmers are people exactly like them--with gigabit connections and unlimited computing power. Poor people living out in the country (or overseas) with slow connections need not apply to participate in this extremely wasteful and broken 'society.' 

(And God forbid you ever attempt to live anymore without a mobile phone. You will be 'persona non grata' everywhere you go, online or off.)

[Michael Catanzaro]

I'm not sure that ranting here will improve your odds of resolution....

(In reply to dave from comment #3)
> It's been broken since before the Blink/WebKit split, and that was in what,
> 2012?

I doubt it's been that long. If you're seeing the same symptom in Chrome, it is unrelated. They never even used our process model, so almost zero chance of sharing a problem like this.

> 'I'm not sure that ranting here will improve your odds of resolution....'

Does ranting anywhere improve anything? If it did, this country would not be the toxic, failed sh_thole that it is, and the world would not be filled with countless angry old men who are pissed off because everything is TRASH.

At this point, I'm halfway convinced the real reason the "stop loading" function does not work is because both WebKit and Chromium developers have conspired to break it, for the explicit purpose of ensuring that it is impossible for the user to abort any malware download that their browser may have triggered. It wouldn't be the first thing Chromium at least pretended "security" while actually making things more insecure.

It's either a conspiracy, or just sheer incompetence and inability to correctly design software, so I'm giving the giving the benefit of the doubt here by alleging conspiracy.

Seriously. How the hell does the "stop download" function FAIL TO STOP ANYTHING, if not SHEER INCOMPETENCE AND/OR MALICIOUS INTENT?

[Michael Catanzaro]

I found two example websites in bug #274727 that continue loading forever, which seemed like a good way to test this bug. On both websites (discourse.gnome.org and www.vox.com), the problem is the same. We get so far as DocumentLoader::stopLoading, and there are three possible ways the load can be canceled. In this case, the main resource is not loading -- it's only a subresource that is still loading -- so we take the second path and call DocumentLoader::setMainDocumentError instead of DocumentLoader::cancelMainResourceLoad. This gets reported back to WebKit layer in WebLocalFrameLoaderClient::setMainDocumentError, which ignores it.

I suspect that Stop has not worked properly since WebKitLegacy (at least).

[Michael Catanzaro]

I was on completely the wrong track. I was looking at WebLocalFrameLoaderClient::dispatchDidFailLoading, but that is used to indicate a particular resource has failed to load. I should have been looking at WebLocalFrameLoaderClient::dispatchDidFailLoad, which indicates the entire page failed to load. That's supposed to be called by FrameLoader::checkLoadCompleteForThisFrame, but isn't happening. There are a variety of reasons why it could fail.

[Michael Catanzaro]

Plot twist: the load actually finishes successfully (at least in the case I'm debugging) and WebKitWebView actually emits the LOAD_FINISHED event, so there's nothing to stop. But the PageLoadStateObserver does not fire the didChangeIsLoading event, so WebKitWebView thinks it is still loading event though the last event was LOAD_FINISHED.

So that's why stop doesn't work. It's not actually loading.

[Michael Catanzaro]

PageLoadState::isLoading returns true if !data.pendingAPIRequest.url.isNull() even if the page load state is finished. It was added in 150676@main to force isLoading to return true immediately after starting a load but before the UI process receives the notification that the load has started. That actually makes sense to me, though. The load in question has already started and finished, so surely there shouldn't still be a pending API request URL.

[Michael Catanzaro]

*** This bug has been marked as a duplicate of bug 274727 ***

[Michael Catanzaro]

Reopening because bug #275178 reveals an actual problem with Stop. When there is a delayed load event, Stop doesn't work until the delayed load event finishes. Surely that shouldn't happen; nothing should ever be able to block Stop.

[Michael Catanzaro]

I naively attempted this:

diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
index 61c2a2e235a0..cff229c32dfa 100644
--- a/Source/WebCore/loader/FrameLoader.cpp
+++ b/Source/WebCore/loader/FrameLoader.cpp
@@ -2714,11 +2714,6 @@ void FrameLoader::checkLoadCompleteForThisFrame(LoadWillContinueInAnotherProcess
 {
     ASSERT(m_client->hasWebView());
 
-    // FIXME: Should this check be done in checkLoadComplete instead of here?
-    // FIXME: Why does this one check need to be repeated here, and not the many others from checkCompleted?
-    if (m_frame->document()->isDelayingLoadEvent())
-        return;
-
     switch (m_state) {
     case FrameState::Provisional: {
         // FIXME: Prohibiting any provisional load failures from being sent to clients
@@ -2732,6 +2727,11 @@ void FrameLoader::checkLoadCompleteForThisFrame(LoadWillContinueInAnotherProcess
         if (!provisionalDocumentLoader)
             return;
 
+        // FIXME: Should this check be done in checkLoadComplete instead of here?
+        // FIXME: Why does this one check need to be repeated here, and not the many others from checkCompleted?
+        if (m_frame->document()->isDelayingLoadEvent() && !m_provisionalDocumentLoader->isStopping())
+            return;
+
         // If we've received any errors we may be stuck in the provisional state and actually complete.
         auto& error = provisionalDocumentLoader->mainDocumentError();
         if (error.isNull())
@@ -2790,6 +2790,11 @@ void FrameLoader::checkLoadCompleteForThisFrame(LoadWillContinueInAnotherProcess
         if (m_documentLoader->isLoadingInAPISense() && !m_documentLoader->isStopping() && !m_checkingLoadCompleteForDetachment)
             return;
 
+        // FIXME: Should this check be done in checkLoadComplete instead of here?
+        // FIXME: Why does this one check need to be repeated here, and not the many others from checkCompleted?
+        if (m_frame->document()->isDelayingLoadEvent() && !m_documentLoader->isStopping())
+            return;
+
         setState(FrameState::Complete);
 
         // FIXME: Is this subsequent work important if we already navigated away?


Unfortunately, removing the isDelayingLoadEvent check from the top of FrameLoader::checkLoadCompleteForThisFrame causes the RELEASE_ASSERT(presentationSize) in VideoFrameGStreamer::createWrappedSample to fail. Not sure why.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12) 
> Unfortunately, removing the isDelayingLoadEvent check from the top of
> FrameLoader::checkLoadCompleteForThisFrame causes the
> RELEASE_ASSERT(presentationSize) in VideoFrameGStreamer::createWrappedSample
> to fail. Not sure why.

OK, it's actually a coincidence. I tested many times with the patch not applied and never saw the crash, and then saw the crash several times with the patch applied, and then removed the patch and didn't see it again a few times. But today it still happens with the patch removed.

[Michael Catanzaro]

Well I don't known what changed (perhaps something on the server?), but I'm seeing this crash almost every time now rather than the problem with Stop not working. So we'll just have to deal with the crash first. Reported as bug #275205