Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Set CT related fields on URLRequestContextStorage #250

Merged
merged 3 commits into from
Sep 29, 2016
Merged

Set CT related fields on URLRequestContextStorage #250

merged 3 commits into from
Sep 29, 2016

Conversation

kevinsawicki
Copy link
Contributor

Follow up fix for #249

Looks like there were a few fields only being set on the net::HttpNetworkSession::Params and not also set on the net::URLRequestContextStorage object that would cause a crash when missing there.

/cc @deepak1556 @sleevi

Refs electron/electron#7389

@sleevi
Copy link

sleevi commented Sep 28, 2016

Alternatively https://cs.chromium.org/chromium/src/net/url_request/url_request_context_builder.h?rcl=0&l=114 may be sufficient. Even if you can't switch wholly to URLRequestContextBuilder, that gives you the 'safe' path for ensuring a URLRequestContext and the Params are kept in sync

@sleevi
Copy link

sleevi commented Sep 28, 2016

This should only cause a crash if you're using the same URLRequestContextStorage between two URLRequestContexts, right?

I don't know the Electron security model, but in Chrome, I flag any such situations as bugs, because they are a routine source of issues. Thinks like ambient auth decisions for 'background' requests leaking and affecting 'foreground' requests. However, that may be totally acceptable for your use case. Just wanted to highlight it (this is why Chrome has a 'system' URL request context and a 'profile' URL request context, and why WebRTC & media use distinct HttpNetworkSessions). That said, it can also cause surprises (e.g. proxies work on main requests, but not for WebRTC). Just wanting to highlight :)

@kevinsawicki
Copy link
Contributor Author

Alternatively https://cs.chromium.org/chromium/src/net/url_request/url_request_context_builder.h?rcl=0&l=114 may be sufficient.

Great suggestion, switched it to use that instead.

@kevinsawicki
Copy link
Contributor Author

This should only cause a crash if you're using the same URLRequestContextStorage between two URLRequestContexts, right?

To be honest, I'm not really sure on this one.

@deepak1556
Copy link
Contributor

This should only cause a crash if you're using the same URLRequestContextStorage between two URLRequestContexts, right?

@sleevi Yeah but in brightray we only allow every BrowserContext to have a single URLRequestContext associated with it (media in the same context shares it), we use this to create session concept similar to profiles of chrome.

@kevinsawicki the change looks good to me.

@zcbenz
Copy link
Contributor

zcbenz commented Sep 29, 2016

👍

@zcbenz zcbenz merged commit 816d787 into master Sep 29, 2016
@zcbenz zcbenz deleted the ct-crash branch September 29, 2016 08:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants