Webpositive crashed WebCore::ResourceHandle::client()

[xray7224]

Web positive crashed. I wasn't doing anything specific just browsing around the web.

I'm using hrev47328 Web positive version: 1.1-alpha HaikuWebKit: 1.3.1 WebKit: 538.33

[xray7224]

debug file

[pulkomandy]

Can you reproduce this in a more recent nightly?

[diver]

crash in hrev48472

[diver]

To reproduce: open ​http://ya.ru and press Alt+r (10-20) times

[jessicah]

This is happening on hrev48819 with haikuwebkit-1.4.10.

To reproduce, go to ​https://gerrit.libreoffice.org/ and click on the login link on the top right. Will crash every time.

[jessicah]

Up to date debug report

[diver]

(In #12047) Dupe of #10924

[markh]

Fix for crash in WebCore::ResourceHandle::client

[markh]

I thought I had fixed it with the patch attached to #12605, but apparently the variable m_resourceHandle can become null in the function HeadersReceived. I now added a patch that moves retrieving the client to right after checking for a valid resourcehandle. This fixes the crash, but I'm not sure why the m_resourceHandle variable becomes null in the function itself.

[pulkomandy]

HeadersReceived is called from the request's own thread. While the request is running, WebKit can decide to abort it. When it does that, it resets the pointer, so the requests crashes (cleanly) instead of corrupting memory. When such crashes happen, moving the things around is not the right thing to do. Instead, the problem should be investigated, and locking added in the right places to avoid such problems.

A big part of the network code in WebKit could be cleaned up. It was derived from the curl code then grew over the years to accomodate changes in our API. I think the whole thing should be reviewed for locking issues.

[markh]

I understand moving things around is not the solution, but doing a review for the whole thing is going to take quite a bit of time I expect, while this crashing issue remains. I would rather have it not crash, while the review is going on.

I would like to look into the locking problem, though. Could you explain how the network code works? Which threads are running and what kind of calls are made to the Haiku networking code? This ticket may not be the right place for it, though.

[markh]

I did some quick debugging to see what is going wrong and it does not seem to be a locking problem. In this case AuthenticationNeeded is called from HeadersReceived and it determines that it is an unknown authentication type and should be ignored. It then calls client->didFinishLoading() and returns. Apparently the call to client->didFinishLoading() destroys the ResourceHandle somewhere, which unsets m_resourceHandle. This then causes the call to m_resourceHandle->client() to fail. Checking if m_resourceHandle is still valid right before this call should fix it. Again, not sure if it is the right fix.

[pulkomandy]

That sounds like an error in AuthenticationNeeded. Unknown authentication types should be passed as-is to the web page and not interrupt the request. Javascript code can then access the HTTP headers and process the authentication request.

As for the services kit design:

Each HTTP request (BHttpRequest) runs in its own thread. It notifies an UrlProtocolListener of all events. In the case of WebKit, an UrlProtocolDispatchingListener is used, which relays the messages to an UrlProtocolAsynchronousListener running in the main thread. This serializes the events so there shouldn't be too much locking problems in this case.

There are some exceptions:

• Some XmlHttpRequests are handled synchronously, which means the main thread is blocked waiting for them to complete. This may cause some of the freezes we get.
• WebSockets are handled with separate code in the WebKit repo, with even more threads (2 per sockets IIRC), and is known to be broken in several cases.

[markh]

Disabling the client->didFinishLoading call when an unknown authentication is encountered fixes the crash and shows me the OpenID login screen for ​https://gerrit.libreoffice.org/, so that looks positive.

Is this always a good idea? What if AuthenticationNeeded is called from somewhere else than HeadersReceived?

[pulkomandy]

I don't think it will be called from elsewhere. Only the HTTP authentication header can trigger it.

[markh]

Fix for handling of unknown authentication type

[markh]

I added a patch that removes the client->didFinishLoading call when an unknown authentication type is encountered. I do wonder about the other calls to didFinishLoading in that function, though. Are they wrong as well?

[pulkomandy]

The idea for the other authentication types was to cancel the request, and avoid downloading the "authentication needed" page content, knowing that the user would request the page again (with authentication data). This is probably not needed anymore, but at least it does not seem to trigger a crash? Or is there some ticket about that too?

[markh]

What I mean is that there are two other places in AuthenticationNeeded that call client->didFinishLoading:

• if m_redirectionTries == 0
• if d->m_user == ""

How bad is that? I am not aware of any issues that relate to those two calls, but if they are triggered it seems to me that this will cause the same kind of crash as we have in this instance.

[pulkomandy]

These two case are supposed to stop the request: The first happens if the browser seems stuck in a redirect loop, The second happens if the user press "cancel" in the HTTP authentication dialog.

In either case, the request should be stopped and not continued with fetching the body. However, the remaining parts of the code should handle this properly, just calling didFinishLoading is not enough as the thread will continue to run for some time.

[markh]

If I understand it correctly, then we still need to check if we want to stop processing after AuthenticationNeeded returns. We could change the following:

        if (statusCode == 401)
            AuthenticationNeeded(httpRequest, response);

into

        if (statusCode == 401)
        {
            AuthenticationNeeded(httpRequest, response);
            if (!m_resourceHandle)
                return;          
       }

[pulkomandy]

Yes, that looks like it could work. Maybe with a comment to explain why the resourceHandle can disappear this way?

[markh]

I tried to make a patch for that, but wanted to test it first. When I now try to compile HaikuLauncher it errors out with:

[2/26] Linking CXX shared library lib/libWebKit.so.1.5.2
FAILED: : && /bin/g++-x86  -fPIC -march=i686  -std=c++11 -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti -frtti  -Wl,--no-undefined -shared -Wl,-soname,libWebKit.so.1 -o lib/libWebKit.so.1.5.2 @CMakeFiles/WebKit.rsp  && :
lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/haiku/RenderThemeHaiku.cpp.o: In function `WebCore::RenderThemeHaiku::paintSliderTrack(WebCore::RenderObject const&, WebCore::PaintInfo const&, WebCore::IntRect const&)':
RenderThemeHaiku.cpp:(.text+0x1149): undefined reference to `WebCore::RenderObject::style() const'
lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/haiku/RenderThemeHaiku.cpp.o: In function `WebCore::RenderThemeHaiku::paintSliderThumb(WebCore::RenderObject const&, WebCore::PaintInfo const&, WebCore::IntRect const&)':
RenderThemeHaiku.cpp:(.text+0x1293): undefined reference to `WebCore::RenderObject::style() const'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

What could be the cause of that?

[jessicah]

Replying to markh:

I tried to make a patch for that, but wanted to test it first. When I now try to compile HaikuLauncher it errors out with:

[2/26] Linking CXX shared library lib/libWebKit.so.1.5.2
FAILED: : && /bin/g++-x86  -fPIC -march=i686  -std=c++11 -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti -frtti  -Wl,--no-undefined -shared -Wl,-soname,libWebKit.so.1 -o lib/libWebKit.so.1.5.2 @CMakeFiles/WebKit.rsp  && :
lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/haiku/RenderThemeHaiku.cpp.o: In function `WebCore::RenderThemeHaiku::paintSliderTrack(WebCore::RenderObject const&, WebCore::PaintInfo const&, WebCore::IntRect const&)':
RenderThemeHaiku.cpp:(.text+0x1149): undefined reference to `WebCore::RenderObject::style() const'
lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/haiku/RenderThemeHaiku.cpp.o: In function `WebCore::RenderThemeHaiku::paintSliderThumb(WebCore::RenderObject const&, WebCore::PaintInfo const&, WebCore::IntRect const&)':
RenderThemeHaiku.cpp:(.text+0x1293): undefined reference to `WebCore::RenderObject::style() const'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

What could be the cause of that?

I recently tried to build haikuwebkit too, and also ran into the same error. I've upgraded my system to gcc5, and am wondering if this is the issue?

[pulkomandy]

Most likely yes, I pushed a fix to the webkit repo for this build problem.

[markh]

Thank, that fixed it. I tested my patch and it seems to be fine. It has a comment indicating why we are checking m_resourceHandle again.

[pulkomandy]

Applied in ​https://github.com/haiku/webkit/commit/10faffadf3d46b293e7bb40d545d8c7de98eb60f.

[diver]

(In #13523) Why do you use such an old Haiku nightly (more than a year old?) Anyway, it's a dupe of #10924 and was fixed long ago :)