Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300453002:
[Extensions] Begin making Extension port initialization asynchronous (Closed)
Description[Extensions] Begin making Extension port initialization asynchronous
Currently, extension port initialization requires a synchronous IPC to
the browser. This is bad, as this can happen during very performance-
sensitive times, such as a content script running at document_start.
Make the port initialization process asynchronous.
To do this, introduce the concept of local vs global ids. A local id is
the id of the port in the javascript context, and the global id is used
by the core extension system. The local id is set synchronously; the
global is set asynchronously. This change also allows us to determine
more quickly whether or not a port exists in a given context.
Because ports are supposed to be able to be used synchronously, also
instrument message caching that will wait until the port is fully
initialized before dispatching any messages.
To begin, this change only modifies the behavior for extension message
ports constructed through runtime.connect(). Ports constructed through
tabs.connect() or runtime.connectNative() will be addressed in
subsequent patches.
This also required updated a number of extensions tests which made
incorrect assumptions about the order of APIs finishing (in particular,
that a port's message would be sent prior to a script executing). This
was never in any way guaranteed.
Drive-by change: Make the extension not having connectNative
permissions in trying to connect to a native app a hard failure.
BUG=642380
Committed: https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3
Cr-Commit-Position: refs/heads/master@{#417784}
Patch Set 1 : Ready #
Total comments: 41
Patch Set 2 : lazyboys #
Total comments: 4
Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Nasko's #Messages
Total messages: 76 (64 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== ports BUG= ========== to ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. BUG=642380 ==========
Description was changed from ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. BUG=642380 ========== to ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. BUG=642380 ==========
rdevlin.cronin@chromium.org changed reviewers: + lazyboy@chromium.org
Hey lazyboy@, mind taking a look? It's a little complicated, but hopefully not too bad.
Overall seems good, I have few comments. Sorry, a bunch of them are from old code I think, but we can clean things up going forward. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... File extensions/renderer/extension_port.h (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:22: // A class to handle sending message port-related IPCs to the browser. Since Also point out what this class represents, "info about a specific port" or sth. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:40: void ClosePort(bool close_channel); "Port" prefix seems extraneous given |this| is a ExtensionPort, just Close() should be enough. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:44: bool initialized() const { return global_id_ != -1; } IsInitialized() and move definition to cc file. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:56: ScriptContext* script_context_ = nullptr; Here and for local_id_: Do you need default member initializer given the constructor always initializes them? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... File extensions/renderer/messaging_bindings.cc (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:39: // var extension = (unrelated), should we remove |extension| from this example? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:57: class RunOnDestruction { Can we use ScopedClosureRunner instead? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:95: bool* port_created, See comment in related to HasMessagePort, I think we should be able to simplify this too. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:300: context_set.ForEach(render_frame, This and HasMessagePort() can be simplified now, right? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:302: // Note: HasMessagePort invokes a JavaScript function. If the runtime of the This isn't true anymore. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:421: int port_id = args[2].As<v8::Int32>()->Value(); local_port_id or local_id? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:450: int local_id = next_local_id_++; I'd wrap this in GetNextLocalId() { return next_local_id_++ } since this is called from many places. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:469: info, channel_name, include_tls_channel_id, generic: Why is |include_tls_channel_id| not part of |info|? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:473: args.GetReturnValue().Set(static_cast<int32_t>(local_id)); Fingers crossed, hope no one is relying on the old global behavior. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:497: // TODO(devlin): Make this async. Bug ID (here and below) https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:515: // - |extension_id| - Extension ID of sender and destination. I find "sender and destination" a bit confusing. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:517: CHECK(args.Length() >= 4 && args[0]->IsInt32() && args[1]->IsInt32() && Curious: why is this >= 4 and not == 4? https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:541: void MessagingBindings::ClosePort(int port_id, bool force_close) { s/port_id/local_port_id https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... File extensions/renderer/messaging_bindings.h (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.h:67: // Creates a new port with the given |global_id|. MessagingBindings owns the port... https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.h:76: using PortMap = std::map<int, std::unique_ptr<ExtensionPort>>; Description: port local id to port info map, etc.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... File extensions/renderer/extension_port.h (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:22: // A class to handle sending message port-related IPCs to the browser. Since On 2016/09/08 00:05:40, lazyboy wrote: > Also point out what this class represents, "info about a specific port" or sth. Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:40: void ClosePort(bool close_channel); On 2016/09/08 00:05:40, lazyboy wrote: > "Port" prefix seems extraneous given |this| is a ExtensionPort, just Close() > should be enough. Fair enough, done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:44: bool initialized() const { return global_id_ != -1; } On 2016/09/08 00:05:40, lazyboy wrote: > IsInitialized() and move definition to cc file. Why? This is basically a simple accessor (it doesn't really do any work) - it just avoids making a bool is_initialized_. In my mind, CamelCase() functions are an indication that some significant work is being done (and e.g. you should cache the result). https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:56: ScriptContext* script_context_ = nullptr; On 2016/09/08 00:05:40, lazyboy wrote: > Here and for local_id_: Do you need default member initializer given the > constructor always initializes them? I think yes, because we want a fallback if the ctor signature changes. I don't know if this ever made it into concrete style, but it's an opinion shared by a few of the folks in the discussion thread: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 "If you use in-class member initialization, then all members that can be value or default initialized must be." (taken from a post, not style, so not dogma) https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... File extensions/renderer/messaging_bindings.cc (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:39: // var extension = On 2016/09/08 00:05:40, lazyboy wrote: > (unrelated), should we remove |extension| from this example? Yep, I have no idea why that was there. Removed. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:57: class RunOnDestruction { On 2016/09/08 00:05:40, lazyboy wrote: > Can we use ScopedClosureRunner instead? That exists? I should have known there are no more original ideas for handy utilities in chromium. ;) Done. I slightly miss the presence of a Reset() that doesn't warn about the release, but ah well. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:95: bool* port_created, On 2016/09/08 00:05:40, lazyboy wrote: > See comment in related to HasMessagePort, I think we should be able to simplify > this too. See reply in HasMessagePort() comment. :) https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:300: context_set.ForEach(render_frame, On 2016/09/08 00:05:40, lazyboy wrote: > This and HasMessagePort() can be simplified now, right? How? I suppose we could just loop over the map of MessagingBindings, but then we have to reimplement the restrict_to_render_frame logic from ScriptContextSet::ForEach() (or check in every binding). I'm not sure either one of those is strictly better or simpler. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:302: // Note: HasMessagePort invokes a JavaScript function. If the runtime of the On 2016/09/08 00:05:40, lazyboy wrote: > This isn't true anymore. Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:421: int port_id = args[2].As<v8::Int32>()->Value(); On 2016/09/08 00:05:40, lazyboy wrote: > local_port_id or local_id? Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:450: int local_id = next_local_id_++; On 2016/09/08 00:05:40, lazyboy wrote: > I'd wrap this in GetNextLocalId() { return next_local_id_++ } since this is > called from many places. Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:469: info, channel_name, include_tls_channel_id, On 2016/09/08 00:05:40, lazyboy wrote: > generic: Why is |include_tls_channel_id| not part of |info|? I really don't know. Added a TODO. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:473: args.GetReturnValue().Set(static_cast<int32_t>(local_id)); On 2016/09/08 00:05:40, lazyboy wrote: > Fingers crossed, hope no one is relying on the old global behavior. Port ids are an implementation detail, exposed only the pseudo-private PortImpl JS object (public documentation [1] doesn't include it). And it seems like all our tests pass. If anyone else was relying on somehow snagging the private field, then I think we're okay breaking them. Of course, there's the possibility that we're relying on it somewhere that isn't tested, but I'm hoping that's not the case. [1] https://developer.chrome.com/extensions/runtime#type-Port https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:497: // TODO(devlin): Make this async. On 2016/09/08 00:05:40, lazyboy wrote: > Bug ID (here and below) Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:515: // - |extension_id| - Extension ID of sender and destination. On 2016/09/08 00:05:40, lazyboy wrote: > I find "sender and destination" a bit confusing. Me too (this was copy-pasted). Replaced with "id of the initiating extension." https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:517: CHECK(args.Length() >= 4 && args[0]->IsInt32() && args[1]->IsInt32() && On 2016/09/08 00:05:40, lazyboy wrote: > Curious: why is this >= 4 and not == 4? Shouldn't be (copy-paste). Updated to be == and also to have separate CHECK statements so we get useful output from any crashes. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:541: void MessagingBindings::ClosePort(int port_id, bool force_close) { On 2016/09/08 00:05:40, lazyboy wrote: > s/port_id/local_port_id Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... File extensions/renderer/messaging_bindings.h (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.h:67: // Creates a new port with the given |global_id|. On 2016/09/08 00:05:40, lazyboy wrote: > MessagingBindings owns the port... Done. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.h:76: using PortMap = std::map<int, std::unique_ptr<ExtensionPort>>; On 2016/09/08 00:05:40, lazyboy wrote: > Description: port local id to port info map, etc. Done, but doc'd over |ports_| (since the typedef isn't restricted to local ids).
lgtm. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... File extensions/renderer/extension_port.h (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:44: bool initialized() const { return global_id_ != -1; } On 2016/09/08 19:15:30, Devlin wrote: > On 2016/09/08 00:05:40, lazyboy wrote: > > IsInitialized() and move definition to cc file. > > Why? This is basically a simple accessor (it doesn't really do any work) - it > just avoids making a bool is_initialized_. In my mind, CamelCase() functions > are an indication that some significant work is being done (and e.g. you should > cache the result). I guess I simplified the meaning of simple accessor, nvm. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/ex... extensions/renderer/extension_port.h:56: ScriptContext* script_context_ = nullptr; On 2016/09/08 19:15:30, Devlin wrote: > On 2016/09/08 00:05:40, lazyboy wrote: > > Here and for local_id_: Do you need default member initializer given the > > constructor always initializes them? > > I think yes, because we want a fallback if the ctor signature changes. I don't > know if this ever made it into concrete style, but it's an opinion shared by a > few of the folks in the discussion thread: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 > > "If you use in-class member initialization, then all members that can be value > or default initialized must be." (taken from a post, not style, so not dogma) Acknowledged. https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... File extensions/renderer/messaging_bindings.cc (right): https://codereview.chromium.org/2300453002/diff/140001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:300: context_set.ForEach(render_frame, On 2016/09/08 19:15:30, Devlin wrote: > On 2016/09/08 00:05:40, lazyboy wrote: > > This and HasMessagePort() can be simplified now, right? > > How? I suppose we could just loop over the map of MessagingBindings, but then > we have to reimplement the restrict_to_render_frame logic from > ScriptContextSet::ForEach() (or check in every binding). I'm not sure either > one of those is strictly better or simpler. I see, I was envisioning a simple "bool HasMessagePort()". ScritpContextSet::ForEach feels heavy given it tries to mitigate against ScriptContext-s going away if we call JavaScript functions within its callbacks. Please ignore this comment as hand rolling a loop with manual check might open a way for future bugs... https://codereview.chromium.org/2300453002/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/executescript/in_frame/test.js (right): https://codereview.chromium.org/2300453002/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/executescript/in_frame/test.js:35: counter++; asertTrue(counter<=5), i.e. catch if it gets called more than 5 times for some reason... https://codereview.chromium.org/2300453002/diff/160001/extensions/renderer/me... File extensions/renderer/messaging_bindings.cc (right): https://codereview.chromium.org/2300453002/diff/160001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:465: CHECK(context()->GetAvailability("runtime.connectNative").is_available()); This used to be a soft failure before. Either document this in CL description or change behavior separately.
Description was changed from ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. BUG=642380 ========== to ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. Drive-by change: Make the extension not having connectNative permissions in trying to connect to a native app a hard failure. BUG=642380 ==========
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2300453002/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/executescript/in_frame/test.js (right): https://codereview.chromium.org/2300453002/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/executescript/in_frame/test.js:35: counter++; On 2016/09/08 21:59:16, lazyboy wrote: > asertTrue(counter<=5), i.e. catch if it gets called more than 5 times for some > reason... Done. https://codereview.chromium.org/2300453002/diff/160001/extensions/renderer/me... File extensions/renderer/messaging_bindings.cc (right): https://codereview.chromium.org/2300453002/diff/160001/extensions/renderer/me... extensions/renderer/messaging_bindings.cc:465: CHECK(context()->GetAvailability("runtime.connectNative").is_available()); On 2016/09/08 21:59:16, lazyboy wrote: > This used to be a soft failure before. > Either document this in CL description or change behavior separately. This should already be handled by the way we route functions (we list it at as a dependent feature), so it should never be hit (theoretically). I've added a comment and updated the CL description.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rdevlin.cronin@chromium.org changed reviewers: + nasko@chromium.org, thestig@chromium.org
+thestig@ for chrome/renderer/chrome_mock_render_thread +nasko@ for extension_messages (less sync IPC!)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
IPC LGTM and some nits. https://codereview.chromium.org/2300453002/diff/180001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2300453002/diff/180001/extensions/common/exte... extensions/common/extension_messages.h:689: int /*port_id, */, nit: /* port_id https://codereview.chromium.org/2300453002/diff/180001/extensions/renderer/ex... File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/2300453002/diff/180001/extensions/renderer/ex... extensions/renderer/extension_frame_helper.cc:193: void ExtensionFrameHelper::OnAssignPortId(int port_id, int request_id) { Methods in cc files should be ordered the same way they are in the header file.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2300453002/diff/180001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2300453002/diff/180001/extensions/common/exte... extensions/common/extension_messages.h:689: int /*port_id, */, On 2016/09/09 21:11:34, nasko (slow) wrote: > nit: /* port_id Done. https://codereview.chromium.org/2300453002/diff/180001/extensions/renderer/ex... File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/2300453002/diff/180001/extensions/renderer/ex... extensions/renderer/extension_frame_helper.cc:193: void ExtensionFrameHelper::OnAssignPortId(int port_id, int request_id) { On 2016/09/09 21:11:34, nasko (slow) wrote: > Methods in cc files should be ordered the same way they are in the header file. Whoops! Thanks for catching this.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, thestig@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2300453002/#ps200001 (title: "Nasko's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. Drive-by change: Make the extension not having connectNative permissions in trying to connect to a native app a hard failure. BUG=642380 ========== to ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. Drive-by change: Make the extension not having connectNative permissions in trying to connect to a native app a hard failure. BUG=642380 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. Drive-by change: Make the extension not having connectNative permissions in trying to connect to a native app a hard failure. BUG=642380 ========== to ========== [Extensions] Begin making Extension port initialization asynchronous Currently, extension port initialization requires a synchronous IPC to the browser. This is bad, as this can happen during very performance- sensitive times, such as a content script running at document_start. Make the port initialization process asynchronous. To do this, introduce the concept of local vs global ids. A local id is the id of the port in the javascript context, and the global id is used by the core extension system. The local id is set synchronously; the global is set asynchronously. This change also allows us to determine more quickly whether or not a port exists in a given context. Because ports are supposed to be able to be used synchronously, also instrument message caching that will wait until the port is fully initialized before dispatching any messages. To begin, this change only modifies the behavior for extension message ports constructed through runtime.connect(). Ports constructed through tabs.connect() or runtime.connectNative() will be addressed in subsequent patches. This also required updated a number of extensions tests which made incorrect assumptions about the order of APIs finishing (in particular, that a port's message would be sent prior to a script executing). This was never in any way guaranteed. Drive-by change: Make the extension not having connectNative permissions in trying to connect to a native app a hard failure. BUG=642380 Committed: https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 Cr-Commit-Position: refs/heads/master@{#417784} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 Cr-Commit-Position: refs/heads/master@{#417784} |
