Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410753003:
Update IPC browser_plugin_instance_id before sending queued messages (Closed)
DescriptionUpdate IPC browser_plugin_instance_id before sending queued messages
Currently, if BrowserPluginGuest receives messages for a BrowserPlugin
prior to attachment, the message may be created with an invalid (0)
value of |browser_plugin_instance_id|. After attachment, when queued
messages are sent, these messages disappear into the ether.
This CL modifies queued IPC messages in BrowserPluginGuest before those
messages are sent, in order to give them a valid instance id.
BUG=534839
Committed: https://crrev.com/6f63918adf16bbd01bba2acb42ca87755fc8f439
Cr-Commit-Position: refs/heads/master@{#354572}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use CHECK() instead of LOG(ERROR). #
Total comments: 2
Patch Set 3 : Move computation outside CHECK(). #
Depends on Patchset: Messages
Total messages: 26 (7 generated)
wjmaclean@chromium.org changed reviewers: + lazyboy@chromium.org, lfg@chromium.org
Opening a new review issue for this work, since this approach is drastically different from the one in https://codereview.chromium.org/1404353004/. Please take a look.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410753003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410753003/1
Overall, I don't like this approach of modifying some binary data without being able to enforce what's inside of that data. If we proceed with this, we should do it a lot of caution, and at least add a big warning on SendMessageToEmbedder() that the first parameter must always be and integer that points to the instance id. I prefer the old approach much better than this one. Do you think we could modify it so that it doesn't use std::function? We could have a base class that contains a virtual function that constructs the message, and SendMessageToEmbedder would take that instead of an IPC::Message. https://codereview.chromium.org/1410753003/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1410753003/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:594: LOG(ERROR) << "Unexpected failure reading remaining IPC::Message payload."; This should never happen, and if it does, we still don't want to send an IPC with the wrong instance id. Also, we need to track if this every fails. We should just have a CHECK() and crash if this fails.
> Overall, I don't like this approach of modifying some > binary data without being able to enforce what's inside of > that data. Agreed, I too think this is sub-optimal. In particular, the reliance on implementation details of the IPC/Pickle classes is worrisome. > If we proceed with this, we should do it a lot of caution, > and at least add a big warning on > SendMessageToEmbedder() that the first parameter must > always be an integer that points to the instance id. Yes, agreed. Sadly, since the Pickle class treats the payload as a buffer of raw bytes, there's no way to detect if some message fails to adhere to the requirements (unless it has a payload of 4 bytes or less, but it could easily be wrong without being that short). > I prefer the old approach much better than this one. Do you > think we could modify it so that it doesn't use > std::function? We could have a base class that > contains a virtual function that constructs the message, > and SendMessageToEmbedder would take that instead of an > IPC::Message. Yes, this approach had actually occured to me ... it's basically a functor class approach. It's a bit messy in that we'd have to derive a separate functor sub-class for every BrowserPluginMsg_X message type, so it's a bit messy. But I'm OK with doing it if people prefer it. https://codereview.chromium.org/1410753003/diff/1/content/browser/browser_plu... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1410753003/diff/1/content/browser/browser_plu... content/browser/browser_plugin/browser_plugin_guest.cc:594: LOG(ERROR) << "Unexpected failure reading remaining IPC::Message payload."; On 2015/10/16 16:26:36, lfg wrote: > This should never happen, and if it does, we still don't want to send an IPC > with the wrong instance id. Also, we need to track if this every fails. We > should just have a CHECK() and crash if this fails. Ok. I had sort of thought that falling back to the existing behaviour and logging an error message might be reasonable, but I'd be OK with a CHECK() to force the failure ... at least that way we would soon enough know if it was happening.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/16 16:50:34, wjmaclean wrote: > > I prefer the old approach much better than this one. Do you > > think we could modify it so that it doesn't use > > std::function? We could have a base class that > > contains a virtual function that constructs the message, > > and SendMessageToEmbedder would take that instead of an > > IPC::Message. > > Yes, this approach had actually occured to me ... it's basically a functor class > approach. It's a bit messy in that we'd have to derive a separate functor > sub-class for every BrowserPluginMsg_X message type, so it's a bit messy. > > But I'm OK with doing it if people prefer it. I just had a chat with Istiaque and he convinced me that this new approach is probably ok (we already read the data here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...).
On 2015/10/16 17:03:57, lfg wrote: > On 2015/10/16 16:50:34, wjmaclean wrote: > > > I prefer the old approach much better than this one. Do you > > > think we could modify it so that it doesn't use > > > std::function? We could have a base class that > > > contains a virtual function that constructs the message, > > > and SendMessageToEmbedder would take that instead of an > > > IPC::Message. > > > > Yes, this approach had actually occured to me ... it's basically a functor > class > > approach. It's a bit messy in that we'd have to derive a separate functor > > sub-class for every BrowserPluginMsg_X message type, so it's a bit messy. > > > > But I'm OK with doing it if people prefer it. > > I just had a chat with Istiaque and he convinced me that this new approach is > probably ok (we already read the data here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...). +1. This approach is generally safe and doesn't expose any new implementation details of IPC. It has always been assumed that the instance ID is the first field in all browserplugin msg.
On 2015/10/16 17:12:00, Fady Samuel wrote: > On 2015/10/16 17:03:57, lfg wrote: > > On 2015/10/16 16:50:34, wjmaclean wrote: > > > > I prefer the old approach much better than this one. Do you > > > > think we could modify it so that it doesn't use > > > > std::function? We could have a base class that > > > > contains a virtual function that constructs the message, > > > > and SendMessageToEmbedder would take that instead of an > > > > IPC::Message. > > > > > > Yes, this approach had actually occured to me ... it's basically a functor > > class > > > approach. It's a bit messy in that we'd have to derive a separate functor > > > sub-class for every BrowserPluginMsg_X message type, so it's a bit messy. > > > > > > But I'm OK with doing it if people prefer it. > > > > I just had a chat with Istiaque and he convinced me that this new approach is > > probably ok (we already read the data here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...). > > +1. This approach is generally safe and doesn't expose any new implementation > details of IPC. It has always been assumed that the instance ID is the first > field in all browserplugin msg. I'm ok with doing this if all of you are, though I would suggest I file a bug to convert this to a functional approach once it's supported on Mac.
Updated, ptal ...
lgtm.
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410753003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410753003/20001
lgtm with one nit. https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.cc:596: CHECK(iter.ReadBytes(&data, remaining_bytes)) It's not a good idea to have the code inside the CHECK() (if someone ever tries to compile without checks, it makes it very hard to find why it doesn't work), please move it outside.
https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.cc:596: CHECK(iter.ReadBytes(&data, remaining_bytes)) On 2015/10/16 18:25:49, lfg wrote: > It's not a good idea to have the code inside the CHECK() (if someone ever tries > to compile without checks, it makes it very hard to find why it doesn't work), > please move it outside. +1 Thanks for noticing this.
The CQ bit was unchecked by wjmaclean@chromium.org
On 2015/10/16 18:27:16, lazyboy wrote: > https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://codereview.chromium.org/1410753003/diff/20001/content/browser/browser... > content/browser/browser_plugin/browser_plugin_guest.cc:596: > CHECK(iter.ReadBytes(&data, remaining_bytes)) > On 2015/10/16 18:25:49, lfg wrote: > > It's not a good idea to have the code inside the CHECK() (if someone ever > tries > > to compile without checks, it makes it very hard to find why it doesn't work), > > please move it outside. > > +1 Thanks for noticing this. I though CHECK()s were always on ... (e.g. look at the use of invert_transform_ in root_window_transformers.cc ...). I'll update it though.
On 2015/10/16 18:31:04, wjmaclean wrote: > I though CHECK()s were always on ... (e.g. look at the use of invert_transform_ > in root_window_transformers.cc ...). I'll update it though. They are always on, I just think it's better practice to avoid important code inside assertion macros.
On 2015/10/16 18:34:23, lfg wrote: > On 2015/10/16 18:31:04, wjmaclean wrote: > > I though CHECK()s were always on ... (e.g. look at the use of > invert_transform_ > > in root_window_transformers.cc ...). I'll update it though. > > They are always on, I just think it's better practice to avoid important code > inside assertion macros. Ahh, ok ... I'm just testing my changes before I upload, so it'll be fixed in a jiffy ...
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/1410753003/#ps40001 (title: "Move computation outside CHECK().")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410753003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410753003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6f63918adf16bbd01bba2acb42ca87755fc8f439 Cr-Commit-Position: refs/heads/master@{#354572} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
