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

Set CT related fields on URLRequestContextStorage #250

Merged
merged 3 commits into from Sep 29, 2016

Conversation

Projects
None yet
4 participants
@kevinsawicki
Member

kevinsawicki commented Sep 28, 2016

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

kevinsawicki 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.

Great suggestion, switched it to use that instead.

@kevinsawicki

This comment has been minimized.

Member

kevinsawicki commented Sep 28, 2016

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

This comment has been minimized.

Contributor

deepak1556 commented Sep 29, 2016

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

This comment has been minimized.

Contributor

zcbenz commented Sep 29, 2016

👍

@zcbenz zcbenz merged commit 816d787 into master Sep 29, 2016

@zcbenz zcbenz deleted the ct-crash branch Sep 29, 2016

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