Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modern HTTP(s) Support #977

Open
wants to merge 63 commits into
base: master
from

Conversation

@TcT2k
Copy link
Contributor

commented Oct 14, 2018

This is my proposal for a simple HTTP request class which would allow modern HTTP and HTTP/s connections based on APIs provided by the operating system.

Please have a look at the interface file to see the complete documentation.

Usage would look like this:

// Create the request object
wxObjectDataPtr<wxWebRequest> request(
    wxWebSession::GetDefault().CreateRequest("https://www.wxwidgets.org/downloads/logos/blocks.png"));
// Bind events
request->Bind(wxEVT_WEBREQUEST_STATE, [](wxWebRequestEvent& evt) {
    switch (evt.GetState())
    {
        // Request completed
        case wxWebRequest::State_Completed:
        {
            wxImage logoImage(*evt->GetResponse()->GetStream());
            if (logoImage.IsOK())
                wxLogInfo("Image loaded");
            break;
        }
        // Request failed
        case wxWebRequest::State_Failed:
            wxLogError("Could not load logo: %s", evt.GetErrorDescription());
            break;
    }
});
// Start the request
request->Start();

As always any feedback is welcome. I don't have an ETA on when or if I would implement this, but I've experience with the proposed underlying APIs and it should be easy to get started.

Initial todo for a basic (but functional) implementation:

  • Interface Documentation
  • Finalize API
    • Storage_File event params
    • Storage_None event params
    • Authentification params and event
  • Add generic wxCredentialEntryDialog
  • Sample application with common usage scenarios
  • WinHTTP implementation
  • NSURLSession implementation
  • libcurl implementation
  • Improve error handling

Further improvements in the future could be:

The following APIs would be used:

Operating SystemAPIHTTPSHTTP/2
Windows WinHTTP Yes Windows 10 1607
macOS NSURLSession macOS 10.9 macOS 10.11
iOS NSURLSession iOS 7.0 iOS 9.0
Linux libcurl Yes 7.47.0

Sample App Screenshots

image
image

@oneeyeman1

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

@TcT2k ,
NSURLSession is available since 10.9. The minimum OSX version for wxWidgets is 10.7.
Is there a fallback for 10.7/10.8?

Also, which *nix libcurl isnot available? Embedded platform? Will this be turned off there? Can configure distinguish such cases?

@eranif

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

Since most of the applications using this suggested API will be a wxWidgets application, doesn't it make more sense to use events?
For example, Send() can fire events when the operation completes, maybe wxEVT_HTTP_DATA_READY and wxEVT_HTTP_ERROR

Also, in order to make this class usable in a secondary thread, a "sync" API should be offered too. (Maybe it can be based on a style bit: wxHTTP_USE_EVENTS?)
When Send() is used in its event mode, it always return true.

A side note:
The name 'Send' implies that we are "sending", while we are actually loading data from the web. So maybe changing it to something like 'Fetch' or 'Load'?

These are my thoughts :)

Copy link
Contributor

left a comment

This looks like an excellent start, thanks a lot!

I can think of many possible extensions, of course, but mostly I think they can be safely postponed until (much) later. One of the things I think we do need to decide from the beginning is whether we need to provide support for any native async APIs because it seems clear to me that after implementing this the next step is to provide some wxHTTPRequestThread that would send and receive HTTP requests and communicate with the main thread via events because the main thread should never perform any network operations directly. We will almost certainly need to have a generic version of this anyhow because I don't think libcurl provides any async APIs (or does it?), but if there is native async support anywhere, we need to ensure that the API is compatible with it. Notably, it should be possible to cancel a request.

To answer @oneeyeman1, we will have wxUSE_HTTPREQUEST, of course, and configure will set it to 0 if libcurl is not found, but this is not the interesting part.

Thanks again Tobias!

interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

@eranif

For example, Send() can fire events when the operation completes, maybe wxEVT_HTTP_DATA_READY and wxEVT_HTTP_ERROR

Sounds like a good idea. To support both usage scenarios it would probably make sense to make two methods like SendAsync and Send.

A side note:
The name 'Send' implies that we are "sending", while we are actually loading data from the web. So maybe changing it to something like 'Fetch' or 'Load'?

Technically we are always sending something (request headers and the URL) and send is also used by something like the XMLHttpRequest in javascript

@TcT2k TcT2k force-pushed the TcT2k:http_request branch from 3b2ad2a to 75368ed Oct 15, 2018
Copy link
Contributor

left a comment

Nothing really important, API still looks good to me.

It would be nice to have an example of async use in the class description too, I think it could be made quite short using Bind().

Thanks!

interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Show resolved Hide resolved
interface/wx/httprequest.h Outdated Show resolved Hide resolved
@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

I've changed to API to async only and changed the structure a little.
Async seems more appropriate and will probably discourage users not to do network request in the main thread.

wxWebRequest are now created by wxWebRequestSession::CreateSession() as a reference counted object instead of their own constructor.

Copy link
Contributor

left a comment

Fixed a few typos, and a couple of other comments.

interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
samples/webrequest/webrequest.cpp Outdated Show resolved Hide resolved
@TcT2k TcT2k force-pushed the TcT2k:http_request branch 2 times, most recently from 4a7599e to 08fcaf2 Oct 17, 2018
Copy link
Contributor

left a comment

A couple more typos

interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

and 2 more typos

interface/wx/webrequest.h Outdated Show resolved Hide resolved
interface/wx/webrequest.h Outdated Show resolved Hide resolved
@TcT2k TcT2k changed the title Modern HTTP(s) Support [Proposal] Modern HTTP(s) Support Oct 19, 2018
@TcT2k TcT2k force-pushed the TcT2k:http_request branch from 946d94a to a2712ad Oct 20, 2018
@mtangoo

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

Not really to add any invaluable comment but I would lie to thank @TcT2k and everyone taking this. I would love to test it when the need be (in Windows, Mac and Linux)

Great work!

@TcT2k TcT2k force-pushed the TcT2k:http_request branch 9 times, most recently from baf9af2 to 508287e Oct 21, 2018
@TcT2k TcT2k force-pushed the TcT2k:http_request branch from 508287e to 4d4fcff Oct 29, 2018
@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I've finalized the API to what I think should work with every purposed backend offered by Windows, macOS and libcurl.
The API is completely async and State change events will always be send in the main thread to allow for easy UI updates from them.

The WinHTTP backend is near feature complete. NSURLSession and CURL are just stubs for now, but I've already modeled some of the API after features offered by NSURLSession.

Like the current WinHTTP backend, the NSURLSession backend will not require an extra thread as the underlying API is async. The CURL backend will probably require one thread per session handling the curl_multi_... and select() calls.

@MaartenBent

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

How will you fix the mingw build? Disable it via configure/cmake when the header/lib is not available, or link dynamic to winhttp?

@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

How will you fix the mingw build? Disable it via configure/cmake when the header/lib is not available, or link dynamic to winhttp?

Disabling is probably the most sensible when building with incomplete headers, like with other semi "modern" windows apis used by wxWidgets.

TcT2k added 15 commits Nov 3, 2018
First incomplete implementation based on NSURLSession
Support older versions might be useful for older
linux distributions and could be used as a
possible fallback to macOS URLSession implementation
on macOS 10.7 and 10.8.
@TcT2k TcT2k force-pushed the TcT2k:http_request branch from afc6be5 to 68cbd54 Dec 10, 2018
@vadz

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Now that 3.1.2 is done, should we merge this (I'll make another review then) or are any (major) changes still planned?

@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

I'm currently not planning major changes. There are a few open points in the NSURLSession implementation (Authentication, build guards against legacy SDKs), but it would probably make sense to merge, as that usually gives more feedback then open pull requests. I've already rebased this on the 3.1.2 release.

@vadz vadz added the high priority label Dec 12, 2018
Copy link
Contributor

left a comment

Sorry, I finally have quite a few comments and I didn't finish the review yet -- but I thought it would be better to submit the part I already did review now to avoid having even more of them later.

Most of the comments are trivial and should be easily dealt with (or I can do it if you prefer when merging, please let me know). However some of them are less so. At the API level the only thing I find really suspicious is SetIgnoreServerErrorStatus(), mentioned in the comments. I also have some doubts about GetStorage() taking an enum: why wouldn't we let it just take a stream directly? With, perhaps, helper functions for creating memory and file streams? But I don't really object to it, just expressing my thoughts...

I only looked at libcurl-based implementation so far (and also common code, but not MSW/Mac implementations nor the sample or the tests) and the main recurrent issue there is that of encoding used. I think we should never use the current locale encoding in this code as it's almost certainly not the right choice for network-facing code.

Again, sorry for the number of comments, but hopefully it won't be too difficult to deal with them -- and if it would, please let me know and let's discuss it. Needless to say, globally this looks pretty great and the comments shouldn't distract from it.

Thanks again for your work!

dnl ------------------------------------------------------------------------

if test "$wxUSE_WEBREQUEST_CURL" != "no"; then
AC_CHECK_HEADER(curl/curl.h,,,[])

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

I think we should try using PKG_CHECK_MODULES first here, for the same reasons as it's now done for libtiff in b9fe8ca

I don't know if we still need to fall back on manual detection, looking at the corresponding file history, it seems to exist for a very long time, so maybe it's not even needed? The documentation could just mention that LIBCURL_{CFLAGS,LIBS} can be set manually before running configure if necessary.

This comment has been minimized.

Copy link
@TcT2k

TcT2k Dec 14, 2018

Author Contributor

My knowledge of autoconf is very limited so I would leave that change open.

In one of the mingws currently in the CI winhttp is missing.

fi

if test "$wxUSE_MSW" = 1; then
dnl TODO: Check for the required headers/libraries under Windows

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

I'd be tempted to just drop this TODO as WinHTTP seems to be supported since MinGW-w64 4.7, but OTOH an AC_CHECK_HEADER(winhttp.h) doesn't cost much neither...

@@ -698,6 +698,11 @@ WX_ARG_FEATURE(sockets, [ --enable-sockets use socket/network clas
WX_ARG_FEATURE(ipv6, [ --enable-ipv6 enable IPv6 support in wxSocket], wxUSE_IPV6)
WX_ARG_FEATURE(ole, [ --enable-ole use OLE classes (Win32 only)], wxUSE_OLE)
WX_ARG_FEATURE(dataobj, [ --enable-dataobj use data object classes], wxUSE_DATAOBJ)
WX_ARG_FEATURE(webrequest, [ --enable-webrequest use wxWebRequest], wxUSE_WEBREQUEST)
WX_ARG_FEATURE(webrequestcurl, [ --enable-webrequest-curl use libcurl with wxWebRequest], wxUSE_WEBREQUEST_LIBCURL)

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

Is the idea for having this as a separate option to allow using it under Mac and MSW too? If so, perhaps we should only set it to true by default under Linux and to false on the platforms where native implementations would normally be used?

This comment has been minimized.

Copy link
@TcT2k

TcT2k Dec 14, 2018

Author Contributor

Yes that's the idea. It might make sense for some scenarios to use curl on Windows or macOS e.g. to support TLS 1.1 on WinXP or for macOS 10.7 and 10.8 that don't support NSURLSession

@@ -253,6 +253,7 @@ This table summarizes some of the global build features affecting the entire
@itemdef{wxUSE_URL_NATIVE, Use native support for some operations with wxURL.}
@itemdef{wxUSE_VALIDATORS, Use wxValidator class.}
@itemdef{wxUSE_VARIANT, Use wxVariant class.}
@itemdef{wxUSE_WEBREQUEST, Use wxWebRequest class.}

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

It's not critical but presumable wxUSE_CREDENTIALDLG should be added here too

void Init(const wxString& message,
const wxString& user,
const wxString& password);
};

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

Minor, but should have wxDECLARE_NO_COPY_CLASS().

wxString::const_iterator it = s.begin();
wxString::const_iterator end = s.end();
while ( it != end && wxIsspace(*it) ) ++it;
while ( it != end && *it != ';' ) value += *it++;

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor
Suggested change
while ( it != end && *it != ';' ) value += *it++;
while ( it != end && *it != ';' )
value += *it++;

void wxWebRequest::ProcessStateEvent(State state, const wxString& failMsg)
{
if (!IsActiveState(state) && GetResponse())

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor
Suggested change
if (!IsActiveState(state) && GetResponse())
if ( !IsActiveState(state) && GetResponse() )

Just for consistency.

//
wxWebResponse::wxWebResponse(wxWebRequest& request) :
m_request(request),
m_readSize(8 * 1024)

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

This looks tiny... I'd use 64KiB if I had to choose arbitrarily, although ideal would be to benchmark it with a few different buffer sizes.

m_request.ReportDataReceived(sizeReceived);

if ( m_request.GetStorage() == wxWebRequest::Storage_File )
m_file.Write(m_readBuffer.GetData(), m_readBuffer.GetDataLen());

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor
Suggested change
m_file.Write(m_readBuffer.GetData(), m_readBuffer.GetDataLen());
{
m_file.Write(m_readBuffer.GetData(), m_readBuffer.GetDataLen());
}
if ( factory != ms_factoryMap.end() )
return factory->second->Create();
else
return NULL;

This comment has been minimized.

Copy link
@vadz

vadz Dec 13, 2018

Contributor

Perhaps we should log an error here?

@TcT2k

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

@vadz I'll address most your comments, before a merge. Some autoconf changes are probably better left to somebody with more autoconf knowledge, but I'll include a TODO where necessary.

Regarding the SetStorage() using an enum: The native NSURLSession api supports these "storage types" directly (even though my currently implementation does not use the native versions yet). If I remember correctly that's especially important for downloading large files.
Additionally I think going this way now will make even more sense in the future to support stuff like background downloads (without the process running) which is also available in NSURLSession and might be available in some future windows API (could already be some a WinRT API available).

Don't worry about the number of comments, it's very helpful to have detailed feedback. And with github it's relatively easy to keep track of them.

@vadz

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

autoconf: no problem, I can change this.

Storage enum: I see, thanks for the explanation (perhaps it would be useful to have a comment in the header summarizing this?), I didn't notice it as I hadn't looked at the Mac implementation yet, sorry. I do agree with this approach after reading the above, thanks again.

Copy link
Contributor

left a comment

Sorry, I ran out of time to completely finish the review today as well, but I think I'm 99% done.

I'm not sure about the state of authentication support under Mac, does it not work at all or does it work only in some scenarios (did I miss this in the docs?)?

I also wonder if it's possible to show a (big) download progress, as this is something that would be relatively often useful, I think.

Thanks again!

class WXDLLIMPEXP_NET wxWebAuthChallengeWinHTTP : public wxWebAuthChallenge
{
public:
explicit wxWebAuthChallengeWinHTTP(Source source, wxWebRequestWinHTTP& request);

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
explicit wxWebAuthChallengeWinHTTP(Source source, wxWebRequestWinHTTP& request);
wxWebAuthChallengeWinHTTP(Source source, wxWebRequestWinHTTP& request);
public:
wxWebRequestWinHTTP(int id, wxWebSessionWinHTTP& session, const wxString& url);

~wxWebRequestWinHTTP();

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
~wxWebRequestWinHTTP();
~wxWebRequestWinHTTP() wxOVERRIDE;
// For MSVC we can link in the required library explicitly, for the other
// compilers (e.g. MinGW) this needs to be done at makefiles level.
#ifdef __VISUALC__
#pragma comment(lib, "Winhttp")

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
#pragma comment(lib, "Winhttp")
#pragma comment(lib, "winhttp")

This is very minor, of course, but I just don't know why would we use a capital letter here when we don't do it anywhere else.


// Helper functions

static wxString wxWinHTTPErrorToString(DWORD errorCode)

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor

This looks very similar to GetSysErrorMsg() from src/common/log.cpp, I think we should have wxMSWFormatMessage() wrapper in wx/msw/private.h that would take HMODULE and replace both of these functions at once.

wxWCharBuffer resBuf(bufferLen);
if ( ::WinHttpQueryHeaders(hRequest, dwInfoLevel, pwszName,
resBuf.data(), &bufferLen, WINHTTP_NO_HEADER_INDEX) )
result.assign(resBuf);

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
result.assign(resBuf);
{
result.assign(resBuf);
}

When the compound statement itself takes more than one line, not putting braces around its body makes it much more difficult to parse IMO.

/// Returns the number of bytes sent to the server.
wxFileOffset GetBytesSent() const;

/// Returns the number of bytes expected to be send to the server.

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
/// Returns the number of bytes expected to be send to the server.
/// Returns the number of bytes expected to be sent to the server.

Could you please clarify in this comment if this is the total number of the remaining number of bytes to be sent?

/// Returns the number of bytes expected to be send to the server.
wxFileOffset GetBytesExpectedToSend() const;

/// Returns the number of bytes received from the server.

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor

Again, I guess this is "received so far", i.e. it's ok to call this before the request completion and the number will increase, is this so?

wxWebRequest::GetAuthChallenge().
Use SetCredentials() to provide user credentials.
*/

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor

Should have @since 3.1.3 too

/**
A wxWebResponse allows access to the response sent by the server.
@since 3.1.2

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
@since 3.1.2
@since 3.1.3
cookies. Additionally, an underlying network connection might be kept
alive to achieve faster additional responses.
@since 3.1.2

This comment has been minimized.

Copy link
@vadz

vadz Dec 14, 2018

Contributor
Suggested change
@since 3.1.2
@since 3.1.3
@mtangoo

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Any progress on this? I would love to drop thrid-party dependency in favor of this one.
I hope it get merged soon.

@mtangoo

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Just pinging on this very useful PR. What is stalling the merge? I at least expected it to be merged given the high priority label on PR. It will be big loss if this code goes unattended until it is no longer marge-able in all practical sense.

Sorry for noises!

@mtangoo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

so I merged and tested under OSX and it sends request then crashes. If I remove call to response->GetHeader() everything works. If I add it, then it crashes

Process 5479 stopped
* thread #12, queue = 'com.apple.CFNetwork.addPersistCacheToStorageDaemon', stop reason = EXC_BAD_ACCESS (code=1, address=0x10240040)
    frame #0: 0x00007fff79259e9d libobjc.A.dylib`objc_msgSend + 29
libobjc.A.dylib`objc_msgSend:
->  0x7fff79259e9d <+29>: andl   0x18(%r10), %r11d
    0x7fff79259ea1 <+33>: shlq   $0x4, %r11
    0x7fff79259ea5 <+37>: addq   0x10(%r10), %r11
    0x7fff79259ea9 <+41>: cmpq   (%r11), %rsi
Target 0: (Studio) stopped.
(lldb) bt
* thread #12, queue = 'com.apple.CFNetwork.addPersistCacheToStorageDaemon', stop reason = EXC_BAD_ACCESS (code=1, address=0x10240040)
  * frame #0: 0x00007fff79259e9d libobjc.A.dylib`objc_msgSend + 29
    frame #1: 0x00007fff51e463f1 CoreFoundation`CFStringCreateCopy + 289
    frame #2: 0x00007fff50fbb934 CFNetwork`_merge_array_string(void const*, void const*, void*) + 82
    frame #3: 0x00007fff51e710e6 CoreFoundation`__CFDictionaryApplyFunction_block_invoke + 22
    frame #4: 0x00007fff51e710a0 CoreFoundation`CFBasicHashApply + 128
    frame #5: 0x00007fff51e70fd0 CoreFoundation`CFDictionaryApplyFunction + 192
    frame #6: 0x00007fff50fbb782 CFNetwork`HTTPHeaderDict::copyAsOrdinaryDict(__CFAllocator const*) const + 72
    frame #7: 0x00007fff50eec6fb CFNetwork`HTTPMessage::copyHeadersWithShadowedArrayValues() + 25
    frame #8: 0x00007fff50df2b9e CFNetwork`URLResponse::createArchiveList(__CFAllocator const*, long*, void const***, long*) + 210
    frame #9: 0x00007fff50df295f CFNetwork`URLResponse::copyPropertyList(__CFAllocator const*) + 61
    frame #10: 0x00007fff50eb6119 CFNetwork`___ZN12__CFURLCache23CreateAndStoreCacheNodeEP16__CFURLCacheNodePK20_CFCachedURLResponsePK10__CFStringPK13_CFURLRequestPKvbRb_block_invoke + 901
    frame #11: 0x00007fff79e4c5fa libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #12: 0x00007fff79e44db8 libdispatch.dylib`_dispatch_client_callout + 8
    frame #13: 0x00007fff79e59217 libdispatch.dylib`_dispatch_queue_serial_drain + 635
    frame #14: 0x00007fff79e4c166 libdispatch.dylib`_dispatch_queue_invoke + 373
    frame #15: 0x00007fff79e59f0d libdispatch.dylib`_dispatch_root_queue_drain_deferred_wlh + 332
    frame #16: 0x00007fff79e5dd21 libdispatch.dylib`_dispatch_workloop_worker_thread + 880
    frame #17: 0x00007fff7a195fd2 libsystem_pthread.dylib`_pthread_wqthread + 980
    frame #18: 0x00007fff7a195be9 libsystem_pthread.dylib`start_wqthread + 13
@mtangoo

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

I tried to dig as deep as I could and here is what I found, the crash originates at this line . Somehow it throws errors. comment it out and the crash is gone.

Tried to put some NSLog as follows:

wxString wxWebResponseURLSession::GetHeader(const wxString& name) const
{
    NSHTTPURLResponse* httpResp = (NSHTTPURLResponse*) m_task.response;
    NSString* value = [httpResp.allHeaderFields objectForKey:wxCFStringRef(name).AsNSString()];
    if (value)
    {
    NSLog(@"HEADERS:%@",httpResp.allHeaderFields);
    NSLog(@"VAL:%@",value);
        return wxCFStringRef(value).AsString();
    }
    else
        return wxString();
}

The result of running sample (with GetHeader() added) is shown below. That is the limit of my understanding on ObjectiveC. Since the app is in early stage We can switch to Curl or other libraries but this was nice addition that doesn't require extra dependency. But since no one is responding to PR, I will consider it abandoned and (reluctantly) shift to something else.

2019-08-12 14:00:03.439443+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.439523+0300 webrequest[24764:744997] VAL:*
2019-08-12 14:00:03.441084+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.441195+0300 webrequest[24764:744997] VAL:true
2019-08-12 14:00:03.443556+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.443654+0300 webrequest[24764:744997] VAL:gzip
2019-08-12 14:00:03.445696+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.445779+0300 webrequest[24764:744997] VAL:160
2019-08-12 14:00:03.447532+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.447595+0300 webrequest[24764:744997] VAL:application/json
2019-08-12 14:00:03.448926+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.448976+0300 webrequest[24764:744997] VAL:Mon, 12 Aug 2019 11:00:03 GMT
2019-08-12 14:00:03.451203+0300 webrequest[24764:744997] HEADERS:{
    "Access-Control-Allow-Credentials" = true;
    "Access-Control-Allow-Origin" = "*";
    Connection = "keep-alive";
    "Content-Encoding" = gzip;
    "Content-Length" = 160;
    "Content-Type" = "application/json";
    Date = "Mon, 12 Aug 2019 11:00:03 GMT";
    "Referrer-Policy" = "no-referrer-when-downgrade";
    Server = nginx;
    "X-Content-Type-Options" = nosniff;
    "X-Frame-Options" = DENY;
    "X-XSS-Protection" = "1; mode=block";
}
2019-08-12 14:00:03.451255+0300 webrequest[24764:744997] VAL:DENY
2019-08-12 14:00:18.626187+0300 webrequest[24764:745130] -[__NSMallocBlock__ length]: unrecognized selector sent to instance 0x101505a80
2019-08-12 14:00:18.627645+0300 webrequest[24764:745130] [General] An uncaught exception was raised
2019-08-12 14:00:18.627691+0300 webrequest[24764:745130] [General] -[__NSMallocBlock__ length]: unrecognized selector sent to instance 0x101505a80
2019-08-12 14:00:18.627856+0300 webrequest[24764:745130] [General] (
	0   CoreFoundation                      0x00007fff51f39d8b __exceptionPreprocess + 171
	1   libobjc.A.dylib                     0x00007fff79267942 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff51fd1164 -[NSObject(NSObject) doesNotRecognizeSelector:] + 132
	3   CoreFoundation                      0x00007fff51eb0d83 ___forwarding___ + 1443
	4   CoreFoundation                      0x00007fff51eb0758 _CF_forwarding_prep_0 + 120
	5   CFNetwork                           0x00007fff50dcbdb0 _CFGregorianDateCreateWithString + 58
	6   CFNetwork                           0x00007fff50dcbd29 _ZL14dateFromStringPK10__CFStringPh + 33
	7   CFNetwork                           0x00007fff50dcbc99 _ZN11URLResponse15getCreationTimeEv + 175
	8   CFNetwork                           0x00007fff50dcbb64 _ZN11URLResponse23calculateHTTPExpirationEv + 30
	9   CFNetwork                           0x00007fff50dcbb0a _ZN11URLResponse17getExpirationTimeEv + 38
	10  CFNetwork                           0x00007fff50dca893 _ZN12HTTPProtocol29validateCachedResponseForLoadEPK20_CFCachedURLResponse + 1539
	11  CFNetwork                           0x00007fff50f180f8 ___ZN12HTTPProtocol40_protocolInterface_isCachedResponseValidEPK20_CFCachedURLResponseP16dispatch_queue_sU13block_pointerFv21CacheResponseValidityE_block_invoke + 29
	12  libdispatch.dylib                   0x00007fff79e4c5fa _dispatch_call_block_and_release + 12
	13  libdispatch.dylib                   0x00007fff79e44db8 _dispatch_client_callout + 8
	14  libdispatch.dylib                   0x00007fff79e59217 _dispatch_queue_serial_drain + 635
	15  libdispatch.dylib                   0x00007fff79e4c166 _dispatch_queue_invoke + 373
	16  libdispatch.dylib                   0x00007fff79e59f0d _dispatch_root_queue_drain_deferred_wlh + 332
	17  libdispatch.dylib                   0x00007fff79e5dd21 _dispatch_workloop_worker_thread + 880
	18  libsystem_pthread.dylib             0x00007fff7a195fd2 _pthread_wqthread + 980
	19  libsystem_pthread.dylib             0x00007fff7a195be9 start_wqthread + 13
)
2019-08-12 14:00:18.630082+0300 webrequest[24764:745130] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[__NSMallocBlock__ length]: unrecognized selector sent to instance 0x101505a80'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff51f39d8b __exceptionPreprocess + 171
	1   libobjc.A.dylib                     0x00007fff79267942 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff51fd1164 -[NSObject(NSObject) doesNotRecognizeSelector:] + 132
	3   CoreFoundation                      0x00007fff51eb0d83 ___forwarding___ + 1443
	4   CoreFoundation                      0x00007fff51eb0758 _CF_forwarding_prep_0 + 120
	5   CFNetwork                           0x00007fff50dcbdb0 _CFGregorianDateCreateWithString + 58
	6   CFNetwork                           0x00007fff50dcbd29 _ZL14dateFromStringPK10__CFStringPh + 33
	7   CFNetwork                           0x00007fff50dcbc99 _ZN11URLResponse15getCreationTimeEv + 175
	8   CFNetwork                           0x00007fff50dcbb64 _ZN11URLResponse23calculateHTTPExpirationEv + 30
	9   CFNetwork                           0x00007fff50dcbb0a _ZN11URLResponse17getExpirationTimeEv + 38
	10  CFNetwork                           0x00007fff50dca893 _ZN12HTTPProtocol29validateCachedResponseForLoadEPK20_CFCachedURLResponse + 1539
	11  CFNetwork                           0x00007fff50f180f8 ___ZN12HTTPProtocol40_protocolInterface_isCachedResponseValidEPK20_CFCachedURLResponseP16dispatch_queue_sU13block_pointerFv21CacheResponseValidityE_block_invoke + 29
	12  libdispatch.dylib                   0x00007fff79e4c5fa _dispatch_call_block_and_release + 12
	13  libdispatch.dylib                   0x00007fff79e44db8 _dispatch_client_callout + 8
	14  libdispatch.dylib                   0x00007fff79e59217 _dispatch_queue_serial_drain + 635
	15  libdispatch.dylib                   0x00007fff79e4c166 _dispatch_queue_invoke + 373
	16  libdispatch.dylib                   0x00007fff79e59f0d _dispatch_root_queue_drain_deferred_wlh + 332
	17  libdispatch.dylib                   0x00007fff79e5dd21 _dispatch_workloop_worker_thread + 880
	18  libsystem_pthread.dylib             0x00007fff7a195fd2 _pthread_wqthread + 980
	19  libsystem_pthread.dylib             0x00007fff7a195be9 start_wqthread + 13
)
libc++abi.dylib: terminating with uncaught exception of type NSException
Process 24764 stopped
* thread #8, queue = 'com.apple.NSURLSession-work', stop reason = signal SIGABRT
    frame #0: 0x00007fff79fceb66 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff79fceb66 <+10>: jae    0x7fff79fceb70            ; <+20>
    0x7fff79fceb68 <+12>: movq   %rax, %rdi
    0x7fff79fceb6b <+15>: jmp    0x7fff79fc5ae9            ; cerror_nocancel
    0x7fff79fceb70 <+20>: retq
Target 0: (webrequest) stopped.
(lldb)
@vadz vadz modified the milestones: 3.1.3, 3.1.4 Sep 15, 2019
@vadz

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

Unfortunately this almost certainly won't be part of 3.1.3. I'd still very much like to have this in 3.2.0, which means it must be included in 3.1.4.

Tobias, do you think you will have time to work on this in the observable future to finish it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.