refactor: create request context from network context #14656

deepak1556 · 2018-09-17T23:50:07Z

Description of Change

Create request context using the network service apis, the service is disabled and will simply be created on the io thread in the browser process. So there should be no change in behavior of these apis. This change is a prerequisite to use the mojo proxy resolver and cookie change notifiers.

Fixes #14441
Fixes #13829
Fixes corrupted net log generation in 3.x and above

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines

Release Notes

Notes: no-notes

deepak1556 · 2018-09-21T09:40:39Z

This PR is ready for review, there are some minor cleanups that will follow up separately. Its easier to review if looked at individual commits, after this change all network code now live under one place in atom/browser/net. Thanks!

nornagon

only a few commits in, but here's what i got so far

atom/browser/atom_browser_client.cc

atom/browser/cookie_change_notifier.cc

nornagon · 2018-09-25T00:02:10Z

atom/browser/cookie_change_notifier.cc

+      content::BrowserContext::GetDefaultStoragePartition(browser_context_)
+          ->GetCookieManagerForBrowserProcess();
+  if (!cookie_manager)
+    return;


Should this be a noisier error? What does a null return from GetCookieManagerForBrowserProcess indicate?

The cookie manager should be initialized whenever a network context is initialized, if there is none then there should be no reason to observe cookie changes in this context. But I can't think of situations where this can happen though.

Perhaps it should be a CHECK then?

Updated it to a CHECK

nornagon · 2018-09-25T00:03:06Z

atom/browser/cookie_change_notifier.cc

+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  binding_.Close();
+  StartListening();


If the cookie service goes down, will this result in us spinning very quickly trying to reconnect to a service that isn't coming back?

Yes the situation needs to be handled, but its not a service yet.

I'm not sure I understand—what isn't a service yet?

atom/browser/net/url_request_context_getter.cc

nornagon · 2018-09-25T00:07:06Z

atom/browser/atom_browser_context.cc

-    return WrapRefCounted(browser_context_map_[key].get());
+  auto* browser_context = browser_context_map_[key].get();
+  if (browser_context)
+    return browser_context;


no need to WrapRefCounted any more?

atom/browser/net/atom_url_request_job_factory.h

nornagon · 2018-09-25T00:09:30Z

atom/browser/net/url_request_context_getter.cc

@@ -76,6 +74,7 @@ network::mojom::NetworkContextParamsPtr CreateDefaultNetworkContextParams(
  network_context_params->accept_language =
      net::HttpUtil::GenerateAcceptLanguageHeader(
          brightray::BrowserClient::Get()->GetApplicationLocale());
+  network_context_params->allow_gssapi_library_load = true;


is this new functionality that we weren't previously enabling?

Nope this has been a default parameter on the HttpAuthHandlerFactory creation and gets enabled indirectly with the old interface, I am just explicitly specifying for this interface.

nornagon · 2018-09-25T00:11:17Z

atom/browser/net/url_request_context_getter.cc

+  job_factory->SetProtocolHandler(url::kAboutScheme,
+                                  std::make_unique<AboutProtocolHandler>());
+  job_factory->SetProtocolHandler(url::kDataScheme,
+                                  std::unique_ptr<net::DataProtocolHandler>());


Why handle these protocols here rather than allowing the network context to handle them (presuming that's what enable_data_url_support = true does)

This is because of the protocol module we provide https://github.com/electron/electron/blob/master/docs/api/protocol.md, to intercept some of the standard schemes with atom/browser/net/atom_url_request_job_factory we need to establish these handlers explicitly so that they can replaced dynamically.

nornagon · 2018-09-25T00:12:02Z

atom/browser/net/url_request_context_getter.cc

+#if !BUILDFLAG(DISABLE_FTP_SUPPORT)
+  auto* host_resolver = url_request_context->host_resolver();
+  job_factory->SetProtocolHandler(
+      url::kFtpScheme, net::FtpProtocolHandler::Create(host_resolver));


is the host_resolver guaranteed to have a longer lifetime than the job_factory here?

zcbenz

Did a rough check of the code and everything seems to be fine.

atom/browser/net/resolve_proxy_helper.cc

nornagon · 2018-10-01T21:37:59Z

atom/browser/api/atom_api_net_log.cc

 }

 void NetLog::StopLogging(mate::Arguments* args) {
-  base::OnceClosure callback;
-  args->GetNext(&callback);
+  args->GetNext(&stop_callback_);


if this gets called twice quickly, then only one of the callbacks will get called.

That's by design with the OnceClosure but I agree it could be problematic if used wrongly.

Maybe a DCHECK(stop_callback_.is_null()); at the front of the func?

Have updated to queue up the callbacks when the logging is not stopped 5fa25ad

nornagon · 2018-10-01T22:23:01Z

spec/api-session-spec.js

+      })
+      server.listen(0, '127.0.0.1', () => {
+        const config = { pacScript: `http://127.0.0.1:${server.address().port}` }
+        session.defaultSession.setProxy(config, () => {


do you need to reset this back to the default config after the test? (or better, use a fresh session?)

Updated to use a custom session.

atom/browser/io_thread.cc

nornagon

Overall LGTM.

ckerr · 2018-10-02T05:14:43Z

atom/browser/api/atom_api_net_log.cc

 }

 void NetLog::StopLogging(mate::Arguments* args) {
-  base::OnceClosure callback;
-  args->GetNext(&callback);
+  args->GetNext(&stop_callback_);


That's by design with the OnceClosure but I agree it could be problematic if used wrongly.

Maybe a DCHECK(stop_callback_.is_null()); at the front of the func?

ckerr · 2018-10-02T05:16:18Z

atom/browser/api/atom_api_net_log.h

@@ -22,15 +27,20 @@ class NetLog : public mate::Wrappable<NetLog> {

  void StartLogging(mate::Arguments* args);
  bool IsCurrentlyLogging();
-  base::FilePath::StringType GetCurrentlyLoggingPath();
+  std::string GetCurrentlyLoggingPath();


(nit) this could be a const method

ckerr · 2018-10-02T05:22:25Z

atom/browser/api/atom_api_net_log.cc

-  net_log_->StopDynamicLogging(std::move(callback));
+  const auto* current_log_state =
+      state.FindKeyOfType("state", base::Value::Type::STRING);
+  if (current_log_state && current_log_state->GetString() == "STOPPING_LOG")


(minor, DRY) this sequence is similar to the IsCurrentlyLogging impl. Might be better to have a std::string GetLoggingState() const that does this and returns the GetString() so that both places can use that code

atom/browser/api/atom_api_session.cc

atom/browser/atom_browser_client.cc

ckerr · 2018-10-02T06:01:36Z

atom/browser/atom_browser_client.cc

+network::mojom::NetworkContextPtr AtomBrowserClient::CreateNetworkContext(
+    content::BrowserContext* browser_context,
+    bool in_memory,
+    const base::FilePath& relative_partition_path) {


(minor) As per style guide, I think in_memory and relative_partition_path should be commented out as unused, e.g.

content::BrowserContext* browser_context, bool /*in_memory*/, const base::FilePath& /*relative_partition_path*/) {

I believe this is only required when the parameter names are omitted which is not in this case ?

I don't understand this comment -- if the name is omitted, then by definition the name can't be commented out? 😃

Unused variables which are obvious from context can have their names omitted entirely, e.g. class Foo { Foo(Foo&&); }; but those not obvious from context should have a name commented out, e.g. void Circle::Rotate(double /*radians*/) {}

ref

Hmm seems like I misread the style guide, will update them. Thanks!

atom/browser/atom_browser_context.cc

atom/browser/atom_browser_context.h

atom/browser/atom_browser_context.cc

ckerr · 2018-10-02T06:21:48Z

atom/browser/io_thread.cc

+  //    for the global requests, like the cert fetchers below and
+  //    geolocation requests.
+  // 3) There is also ongoing work in upstream which will eventually allow
+  //    localizing these global fetchers to their own URLRequestContexts.


🎉

the breadcrumbs & rationale are appreciated

deepak1556 · 2018-10-02T08:52:28Z

@nornagon @ckerr have updated the PR to address the review comments. Thanks!

nitsakh

LGTM 👍 !

nitsakh · 2018-10-02T21:19:11Z

patches/common/chromium/gritsettings_resource_ids.patch

+     "includes": [28750],
+   },
+  "electron/electron_resources.grd": {
+    "includes": [31750],


Other than the fact that the number should be greater than 31000, no special reason for this number, right?

Yup that is the only reason.

nitsakh · 2018-10-02T21:32:03Z

atom/browser/api/atom_api_net_log.cc


-  net_log_->StopDynamicLogging(std::move(callback));
+  if (stop_callback_queue_.empty())


This doesn't matter right now, because we're not doing much here; but ☝️ should be checked inside the if block 👇 in the stop logging case or it can be removed.

nitsakh · 2018-10-02T21:33:22Z

atom/browser/net/url_request_context_getter.cc

+      protocol_interceptors_.clear();
+    }
+    top_job_factory_->Chain(std::move(inner_job_factory));
+    main_request_context_->set_job_factory(top_job_factory_.get());


The ownership is transferred here. How will the lifetimes work for top_job_factory_?

Its not transferred, URLRequestContext only has weak references to its members, any reason to think the ownership was transferred here ?

nitsakh · 2018-10-02T21:37:12Z

atom/browser/api/atom_api_net_log.cc

+      return current_log_path->GetString();
+  }
+
+  return std::string();
 }

 void NetLog::StopLogging(mate::Arguments* args) {
  base::OnceClosure callback;
  args->GetNext(&callback);


I'm not sure if the callback not provided case is taken care of here.

Its actually checked on the js side atom/browser/lib/net-log.js, could have moved that code here but its better to do that check on js side avoiding an extra js<->c++ type conversion

nitsakh · 2018-10-02T21:47:07Z

atom/browser/atom_browser_context.cc

+std::unique_ptr<content::ZoomLevelDelegate>
+AtomBrowserContext::CreateZoomLevelDelegate(
+    const base::FilePath& partition_path) {
+  if (!IsOffTheRecord()) {


For my understanding, what does this if condition mean, and why do we need it?

IsOffTheRecord is the incognito mode in Electron, we don't persist any of the user preferences to file system when the app exits. Users will get this mode when they create a temporary partition for a session.

…urce

release-clerk · 2018-10-04T18:09:00Z

No Release Notes

* [ci skip] refactor: create request context from network context * [ci skip] refactor: subscribe to mojo cookiemanager for cookie changes * [ci skip] refactor: manage the lifetime of custom URLRequestJobFactory * refactor: use OOP mojo proxy resolver * revert: add support for kIgnoreCertificateErrorsSPKIList * build: provide service manifest overlays for content services * chore: gn format * fix: log-net-log switch not working as expected * spec: verify proxy settings are respected from pac script with session.setProxy * chore: use chrome constants where possible * fix: initialize request context for global cert fetcher * refactor: fix destruction of request context getters * spec: use custom session for proxy tests * fix: queue up additional stop callbacks while net log is being stopped * fix: Add CHECK for cookie manager retrieval * chore: add helper to retrieve logging state for net log module * fix: ui::ResourceBundle::GetRawDataResourceForScale => GetRawDataResource * style: comment unused parameters * build: move //components/certificate_transparency deps from //brightray * chore: update gritsettings_resource_ids patch * chore: update api for chromium 68 * fix: net log instance is now a property of session

Backports #14656

talon266 · 2018-12-05T10:38:52Z

Hey guys!
First of all thanks for this fix and cudos to all who made it happen.
Is there an estimation on the release of this fix? we sure could use it.

Thanks again.

deepak1556 · 2018-12-05T11:54:31Z

@tallev266 3.1.0-beta.x has this fix and also 4.x release line.

talon266 · 2018-12-05T12:01:39Z

@deepak1556 - Thanks! I guess we'll wait for the RC then

bughit · 2018-12-05T16:32:26Z

@deepak1556 #14411 is fixed in 3.1?

bughit · 2018-12-05T16:44:50Z

@deepak1556 #14411 is fixed in 3.1?

already in v3.1.0-beta.2 or in next release?

deepak1556 requested a review from a team September 17, 2018 23:50

deepak1556 force-pushed the network_service_patch branch 3 times, most recently from 50c448e to 2ce9271 Compare September 21, 2018 08:55

deepak1556 changed the title ~~[WIP] refactor: create request context from network context~~ refactor: create request context from network context Sep 21, 2018

deepak1556 requested review from zcbenz, MarshallOfSound, nornagon, alexeykuzmin and ckerr September 21, 2018 09:38

deepak1556 force-pushed the network_service_patch branch 2 times, most recently from 7d04f1f to 440296b Compare September 23, 2018 02:19

alexeykuzmin added the target/3-0-x label Sep 24, 2018

nornagon reviewed Sep 26, 2018

View reviewed changes

zcbenz approved these changes Sep 28, 2018

View reviewed changes

nornagon reviewed Oct 1, 2018

View reviewed changes

atom/browser/net/resolve_proxy_helper.cc Show resolved Hide resolved

nornagon reviewed Oct 1, 2018

View reviewed changes

atom/browser/io_thread.cc Show resolved Hide resolved

nornagon approved these changes Oct 1, 2018

View reviewed changes

MarshallOfSound added target/3-0-x and removed target/3-0-x labels Oct 2, 2018

ckerr reviewed Oct 2, 2018

View reviewed changes

deepak1556 force-pushed the network_service_patch branch from 440296b to d27352d Compare October 2, 2018 07:40

nitsakh approved these changes Oct 2, 2018

View reviewed changes

ckerr approved these changes Oct 3, 2018

View reviewed changes

deepak1556 force-pushed the network_service_patch branch from d27352d to 915cd4a Compare October 3, 2018 19:51

[ci skip] refactor: create request context from network context

fd181e7

deepak1556 added 9 commits October 4, 2018 21:39

fix: queue up additional stop callbacks while net log is being stopped

0f8ae35

fix: Add CHECK for cookie manager retrieval

5fb4738

chore: add helper to retrieve logging state for net log module

6df20d3

fix: ui::ResourceBundle::GetRawDataResourceForScale => GetRawDataReso…

8f53353

…urce

style: comment unused parameters

461b0dd

build: move //components/certificate_transparency deps from //brightray

61fbc65

chore: update gritsettings_resource_ids patch

a8053c1

chore: update api for chromium 68

7a02d6c

fix: net log instance is now a property of session

cd58f06

deepak1556 force-pushed the network_service_patch branch from 915cd4a to cd58f06 Compare October 4, 2018 16:10

deepak1556 requested a review from a team October 4, 2018 16:10

alexeykuzmin added target/3-0-x and removed target/3-0-x labels Oct 4, 2018

ckerr merged commit 434a6e3 into master Oct 4, 2018

ckerr deleted the network_service_patch branch October 4, 2018 18:09

sofianguy added the app-feedback-program label Oct 10, 2018

deepak1556 mentioned this pull request Oct 16, 2018

3.0 regression: crash on exit after https tunneling through a proxy requiring negotiate auth #14411

Closed

deepak1556 mentioned this pull request Oct 24, 2018

fix: proxy resolution from pac file for ws requests on windows and macOS (3-0-x) electron/libchromiumcontent#696

Merged

4 tasks

deepak1556 mentioned this pull request Nov 6, 2018

Slow/weird network requests #13829

Closed

deepak1556 mentioned this pull request Nov 21, 2018

Enabling network service #15791

Closed

15 tasks

deepak1556 added a commit that referenced this pull request Nov 23, 2018

refactor: create request context from network context

e2d939c

Backports #14656

deepak1556 added a commit that referenced this pull request Nov 23, 2018

refactor: create request context from network context

d7136bf

Backports #14656

deepak1556 mentioned this pull request Nov 23, 2018

fix: switch to mojo proxy resolver (3-1-x) #15813

Merged

5 tasks


		net_log_->StopDynamicLogging(std::move(callback));
		if (stop_callback_queue_.empty())

refactor: create request context from network context #14656

refactor: create request context from network context #14656

Conversation

deepak1556 commented Sep 17, 2018 • edited

Description of Change

Checklist

Release Notes

deepak1556 commented Sep 21, 2018

nornagon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak1556 Sep 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak1556 commented Oct 2, 2018 • edited

nitsakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

release-clerk bot commented Oct 4, 2018

talon266 commented Dec 5, 2018

deepak1556 commented Dec 5, 2018

talon266 commented Dec 5, 2018

bughit commented Dec 5, 2018

bughit commented Dec 5, 2018

deepak1556 commented Sep 17, 2018 •

edited

deepak1556 Sep 26, 2018 •

edited

deepak1556 commented Oct 2, 2018 •

edited