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

net: catch source stream creation failure for content encoding #9001

Merged
merged 1 commit into from Mar 28, 2017

Conversation

Projects
None yet
2 participants
@deepak1556
Member

deepak1556 commented Mar 24, 2017

This is the only scenario where URLRequest::Delegate::OnResponseStarted will not be notified before URLRequest::Delegate::OnReadCompleted. https://codereview.chromium.org/2451233002 , made that if there is no sdch implementation then request should fail with net::ERR_CONTENT_DECODING_INIT_FAILED but this is fixed later on in https://codereview.chromium.org/2512263002 where raw response body is passed through for sdch when it is not implemented just like other unsupported content encoding types, we should get the fix probably with chrome 57 or 58 upgrade. For the same reason its not possible to test this scenario as the test case in the original issue will no longer fail with the above fix, but its good to catch this scenario.

Fixes #8867

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 27, 2017

Contributor

@deepak1556 would it be possible to add a spec for this?

Contributor

kevinsawicki commented Mar 27, 2017

@deepak1556 would it be possible to add a spec for this?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Mar 27, 2017

Member

I can add a spec based on the example from the original issue, but that will start to fail once we get https://codereview.chromium.org/2512263002 . The only reliable way to trigger a source stream creation failure is on the native side similar to https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job_unittest.cc?l=123-143 . I wasn't sure of creating a mock c++ test class just for this scenario.

Member

deepak1556 commented Mar 27, 2017

I can add a spec based on the example from the original issue, but that will start to fail once we get https://codereview.chromium.org/2512263002 . The only reliable way to trigger a source stream creation failure is on the native side similar to https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job_unittest.cc?l=123-143 . I wasn't sure of creating a mock c++ test class just for this scenario.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Mar 27, 2017

Member

Also we wonn't mostly hit this scenario in the future unless some content encoding requires implementation or otherwise failure case.

Member

deepak1556 commented Mar 27, 2017

Also we wonn't mostly hit this scenario in the future unless some content encoding requires implementation or otherwise failure case.

@kevinsawicki kevinsawicki merged commit 03b2167 into master Mar 28, 2017

5 of 6 checks passed

electron-win-ia32 Build #2708 failed in 11 min
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #3704 succeeded in 8 min 3 sec
Details
electron-osx-x64 Build #3704 succeeded in 9 min 10 sec
Details
electron-win-x64 Build #2679 succeeded in 8 min 15 sec
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 28, 2017

Contributor

Thanks for fixing this 👍

Contributor

kevinsawicki commented Mar 28, 2017

Thanks for fixing this 👍

@kevinsawicki kevinsawicki deleted the net_source_stream_error_patch branch Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment