Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145463002:
Extensions: Send the tab id to platform apps. (Closed)
|
Created:
6 years, 11 months ago by Lei Zhang Modified:
6 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, vandebo (ex-Chrome), tommycli, Greg Billock, chromium-apps-reviews_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionExtensions: Send the tab id to platform apps.
This lets mediaGalleries.addUserSelectedFolder() figure out the tab
that triggered the API, so it can display the select dialog in the tab
when the platform app has no open windows.
BUG=333899
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249322
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : get rid of js changes, correctly handle disconnects #
Total comments: 2
Patch Set 6 : rebase to head #Patch Set 7 : fix nit #Patch Set 8 : rebase, fix failing dcheck #Patch Set 9 : revert some bad cleanups to fix failing tests #Patch Set 10 : fix null pointer deref #Messages
Total messages: 37 (0 generated)
This seems to work, but I really don't know what goes where. Getting the tab id to chrome/renderer/extensions/request_sender.cc:95 was quite an adventure.
Some fairly high level comments. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:411: // whether this API can access the source tab or not. I'm being dense and can't follow why this (and the corresponding CanPlatformAppAccessTab method) is needed. Is it so that the browser doesn't need to trust what the renderer says re tab ID or something? If it is that... I wouldn't worry about it, we already trust the information about the sender extension ID. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/messaging/message_service.cc:299: if (source_contents) { Maybe we should just always send the tab data and filter it out when passing to the app (on the renderer). Seems like that would save a lot of complexity in this file? i.e. here https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... check both source_tab.empty() and !extension->is_platform_app(). Maybe the source_tab.empty() check isn't even needed. ... and as I read more of this CL it looks like you are doing this filtering in the JavaScript code anyway, so you might as well always send it. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/messaging/message_service.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/messaging/message_service.h:162: bool CanPlatformAppAccessTab(const std::string app_id, int tab_id) const; const std::string& https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_tab_util.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_tab_util.h:73: // Creates a Tab object with only the tab id for platform apps. If you did the filtering on the renderer would you need this? These tab values aren't actually exposed to the app so it doesn't really matter what value they have, right, only that the tab ID is propagated back through to the API call. https://codereview.chromium.org/145463002/diff/1/chrome/common/extensions/ext... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/common/extensions/ext... chrome/common/extensions/extension_messages.h:85: // The id of the tab that sent this request, if any. Comment that this is -1 if there isn't a tab ID source. https://codereview.chromium.org/145463002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/messaging.js (right): https://codereview.chromium.org/145463002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/messaging.js:231: tlsChannelId) { you could pass the isPlatformApp flag in here and save having to store it in the sourceTab structure (you can retrieve it from the extension() value when you run the dispatchOnConnect function from messaging_natives.cc) https://codereview.chromium.org/145463002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/messaging.js:264: isExternal, senderTabId); The above comment said... maybe it would be easier to implement this from C++ similar to https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... -- you'd need to define your own ScopedSenderTabID type or something, but I'd feel more confident that it was correct. Changing this JS file is scary, it's had so many subtle bugs over its time. If you do that in C++ then I'm not even sure you need to change any JS.
Just responding to some of your comments. I'll ping you when I upload a new patch set. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:411: // whether this API can access the source tab or not. On 2014/01/23 16:41:59, kalman wrote: > I'm being dense and can't follow why this (and the corresponding > CanPlatformAppAccessTab method) is needed. Is it so that the browser doesn't > need to trust what the renderer says re tab ID or something? If it is that... I > wouldn't worry about it, we already trust the information about the sender > extension ID. Yes, I'm being paranoid, but if you say we don't need it, I'll remove it. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/messaging/message_service.cc:299: if (source_contents) { On 2014/01/23 16:41:59, kalman wrote: > Maybe we should just always send the tab data and filter it out when passing to > the app (on the renderer). Seems like that would save a lot of complexity in > this file? > > i.e. here > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > check both source_tab.empty() and !extension->is_platform_app(). Maybe the > source_tab.empty() check isn't even needed. > > ... and as I read more of this CL it looks like you are doing this filtering in > the JavaScript code anyway, so you might as well always send it. Again, I'm doing the filter on the browser side because I'm being paranoid. We can certainly do the filtering on the extension side. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/messaging/message_service.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/messaging/message_service.h:162: bool CanPlatformAppAccessTab(const std::string app_id, int tab_id) const; On 2014/01/23 16:41:59, kalman wrote: > const std::string& It sounds like CanPlatformAppAccessTab() isn't needed, so this will just go away. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_tab_util.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_tab_util.h:73: // Creates a Tab object with only the tab id for platform apps. On 2014/01/23 16:41:59, kalman wrote: > If you did the filtering on the renderer would you need this? These tab values > aren't actually exposed to the app so it doesn't really matter what value they > have, right, only that the tab ID is propagated back through to the API call. If I did the filtering on the renderer side, then this is not needed. https://codereview.chromium.org/145463002/diff/1/chrome/common/extensions/ext... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/145463002/diff/1/chrome/common/extensions/ext... chrome/common/extensions/extension_messages.h:85: // The id of the tab that sent this request, if any. On 2014/01/23 16:41:59, kalman wrote: > Comment that this is -1 if there isn't a tab ID source. Will do.
https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:411: // whether this API can access the source tab or not. On 2014/01/23 20:53:12, Lei Zhang wrote: > On 2014/01/23 16:41:59, kalman wrote: > > I'm being dense and can't follow why this (and the corresponding > > CanPlatformAppAccessTab method) is needed. Is it so that the browser doesn't > > need to trust what the renderer says re tab ID or something? If it is that... > I > > wouldn't worry about it, we already trust the information about the sender > > extension ID. > > Yes, I'm being paranoid, but if you say we don't need it, I'll remove it. Thanks for being paranoid. The bug, if it can be called that, with the extension ID may be worth fixing at some point; and it would make sense to include the source tab ID information with that. So long as it's C++ not JS making the decisions on the renderer I'm ok with it though. JS is a different story. Never trust. https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/messaging/message_service.cc:299: if (source_contents) { On 2014/01/23 20:53:12, Lei Zhang wrote: > On 2014/01/23 16:41:59, kalman wrote: > > Maybe we should just always send the tab data and filter it out when passing > to > > the app (on the renderer). Seems like that would save a lot of complexity in > > this file? > > > > i.e. here > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > > check both source_tab.empty() and !extension->is_platform_app(). Maybe the > > source_tab.empty() check isn't even needed. > > > > ... and as I read more of this CL it looks like you are doing this filtering > in > > the JavaScript code anyway, so you might as well always send it. > > Again, I'm doing the filter on the browser side because I'm being paranoid. We > can certainly do the filtering on the extension side. I don't think accidentally giving the tab info to apps is really a big deal (for a very long time we did actually do this). We strip out the sensitive parts without the tab permission -- which should always be the case for apps.
PTAL at patch set 2. I've minimized the JS changes and tried to address most of your comments. I couldn't do the ScopedSenderTabID idea because the push/pop commands needs to be done in MessagingBindings::DispatchOnConnect() and DispatchOnDisconnect(), respectively.
lg but want to make sure about the scoped thing. Assuming my mental model of this stuff is right (... not necessarily true) then it would cut down on quite a few files that you've needed to change on the renderer side to only a couple. https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:416: bool found_tab = extensions::ExtensionTabUtil::GetTabById( Comment like "If the request originated from a tab show the confirmation dialogue in that tab, not the background page". https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:306: source_url_for_tab = source_url; I don't think this source_url_for_tab is necessary any more? It only existed so that we didn't send the URL to the renderer for apps, but now we do. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/media_galleries_custom_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/media_galleries_custom_bindings.cc:89: if (args.Length() != 0) { Just CHECK here. If the JavaScript calls the function with the wrong parameters then we really want to know (and FWIW I've tracked down bugs caused by exactly this behaviour). https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/messaging_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:274: if (!source_tab.empty() && (*it)->extension()->is_platform_app()) shouldn't this be !is_platform_app() ? https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:322: MediaGalleriesCustomBindings::PushSenderTabId(*it, tab_id); Having this in MediaGalleriesCustomBindings is odd since it's a generic messaging functionality. Just put the push/pop logic in this class. I don't understand why you can't have this scoped thing though. The function call needs to be made synchronously during sendRequest so it will happen within the CallModuleMethod call below. That could set a global variable somewhere, with an implicit stack (from the scoped thing saving/restoring the existing tab ID value) to read out of. Sorry if I'm missing something here. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/request_sender.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/request_sender.cc:60: int source_tab_id, yeah so (assuming I'm thinking about this right) the ScopedTabId class would be on RequestSender. It would would read that (class-) global variable below when setting params.source_tab_id rather than passing it in here.
https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:416: bool found_tab = extensions::ExtensionTabUtil::GetTabById( On 2014/01/24 21:37:27, kalman wrote: > Comment like "If the request originated from a tab show the confirmation > dialogue in that tab, not the background page". Done. https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:306: source_url_for_tab = source_url; On 2014/01/24 21:37:27, kalman wrote: > I don't think this source_url_for_tab is necessary any more? It only existed so > that we didn't send the URL to the renderer for apps, but now we do. Under |kUrlKey| right? I removed |source_url_for_tab| and read out of |source_tab| in MessageService::OpenChannelImpl(). If you prefer to do it elsewhere, let me know. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/media_galleries_custom_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/media_galleries_custom_bindings.cc:89: if (args.Length() != 0) { On 2014/01/24 21:37:27, kalman wrote: > Just CHECK here. If the JavaScript calls the function with the wrong parameters > then we really want to know (and FWIW I've tracked down bugs caused by exactly > this behaviour). Done. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/messaging_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:274: if (!source_tab.empty() && (*it)->extension()->is_platform_app()) On 2014/01/24 21:37:27, kalman wrote: > shouldn't this be !is_platform_app() ? Whoops. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:322: MediaGalleriesCustomBindings::PushSenderTabId(*it, tab_id); On 2014/01/24 21:37:27, kalman wrote: > Having this in MediaGalleriesCustomBindings is odd since it's a generic > messaging functionality. Just put the push/pop logic in this class. Sure, moved here. > I don't understand why you can't have this scoped thing though. The function > call needs to be made synchronously during sendRequest so it will happen within > the CallModuleMethod call below. That could set a global variable somewhere, > with an implicit stack (from the scoped thing saving/restoring the existing tab > ID value) to read out of. > > Sorry if I'm missing something here. Are you assuming the API is synchronous? Media Galleries APIs are async.
https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:306: source_url_for_tab = source_url; On 2014/01/25 01:47:09, Lei Zhang wrote: > On 2014/01/24 21:37:27, kalman wrote: > > I don't think this source_url_for_tab is necessary any more? It only existed > so > > that we didn't send the URL to the renderer for apps, but now we do. > > Under |kUrlKey| right? I removed |source_url_for_tab| and read out of > |source_tab| in MessageService::OpenChannelImpl(). If you prefer to do it > elsewhere, let me know. Actually not quite what I meant but what you did is better, and in fact you could remove the source URL all the way down through ExtensionMessagePort and on the IPC layer. However perhaps just leave it as a TODO for now and stick with what you have. https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/messaging_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:322: MediaGalleriesCustomBindings::PushSenderTabId(*it, tab_id); > > I don't understand why you can't have this scoped thing though. The function > > call needs to be made synchronously during sendRequest so it will happen > within > > the CallModuleMethod call below. That could set a global variable somewhere, > > with an implicit stack (from the scoped thing saving/restoring the existing > tab > > ID value) to read out of. > > > > Sorry if I'm missing something here. > > Are you assuming the API is synchronous? Media Galleries APIs are async. No, it's an async API, but running the callback itself is synchronous and that's what matters(?): chrome.runtime.onMessageExternal.addListener(function() { // user action = true, tab ID = 42 chrome.mediaGalleries.doSomething(function() { }); // user action = false, tab ID = undefined }); i.e. the chrome.mediaGalleries.doSomething call is executed with that user action / tab ID state because the event listener has it scoped, so if you read it from the generic SendRequest place it will be set. Sorry if I'm missing something.
https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:306: source_url_for_tab = source_url; On 2014/01/27 17:51:46, kalman wrote: > On 2014/01/25 01:47:09, Lei Zhang wrote: > > On 2014/01/24 21:37:27, kalman wrote: > > > I don't think this source_url_for_tab is necessary any more? It only existed > > so > > > that we didn't send the URL to the renderer for apps, but now we do. > > > > Under |kUrlKey| right? I removed |source_url_for_tab| and read out of > > |source_tab| in MessageService::OpenChannelImpl(). If you prefer to do it > > elsewhere, let me know. > > Actually not quite what I meant but what you did is better, and in fact you > could remove the source URL all the way down through ExtensionMessagePort and on > the IPC layer. However perhaps just leave it as a TODO for now and stick with > what you have. Yes, I noticed the rabbit hole is quite deep and I stopped here to keep this CL small. I added a TODO in MessageService::OpenChannelImpl(). https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/messaging_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:322: MediaGalleriesCustomBindings::PushSenderTabId(*it, tab_id); On 2014/01/27 17:51:46, kalman wrote: > > > I don't understand why you can't have this scoped thing though. The function > > > call needs to be made synchronously during sendRequest so it will happen > > within > > > the CallModuleMethod call below. That could set a global variable somewhere, > > > with an implicit stack (from the scoped thing saving/restoring the existing > > tab > > > ID value) to read out of. > > > > > > Sorry if I'm missing something here. > > > > Are you assuming the API is synchronous? Media Galleries APIs are async. > > No, it's an async API, but running the callback itself is synchronous and that's > what matters(?): > > chrome.runtime.onMessageExternal.addListener(function() { > // user action = true, tab ID = 42 > chrome.mediaGalleries.doSomething(function() { > }); > // user action = false, tab ID = undefined > }); > > i.e. the chrome.mediaGalleries.doSomething call is executed with that user > action / tab ID state because the event listener has it scoped, so if you read > it from the generic SendRequest place it will be set. > > Sorry if I'm missing something. Ah I see what you mean. I'll try to narrow the scope down and see if we can get it to a point where we can make it scoped. If you happen to know where that is, that would be very helpful.
jww: I think we are just trying to narrow down where a couple of calls are being made, so the overall CL isn't going to change much. In http://code.google.com/p/chromium/issues/detail?id=333899#c19 you said this is likely ok, so I'd like to get your sign-off on this.
https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... File chrome/renderer/extensions/messaging_bindings.cc (right): https://codereview.chromium.org/145463002/diff/190001/chrome/renderer/extensi... chrome/renderer/extensions/messaging_bindings.cc:322: MediaGalleriesCustomBindings::PushSenderTabId(*it, tab_id); On 2014/01/27 23:19:13, Lei Zhang wrote: > On 2014/01/27 17:51:46, kalman wrote: > > > > I don't understand why you can't have this scoped thing though. The > function > > > > call needs to be made synchronously during sendRequest so it will happen > > > within > > > > the CallModuleMethod call below. That could set a global variable > somewhere, > > > > with an implicit stack (from the scoped thing saving/restoring the > existing > > > tab > > > > ID value) to read out of. > > > > > > > > Sorry if I'm missing something here. > > > > > > Are you assuming the API is synchronous? Media Galleries APIs are async. > > > > No, it's an async API, but running the callback itself is synchronous and > that's > > what matters(?): > > > > chrome.runtime.onMessageExternal.addListener(function() { > > // user action = true, tab ID = 42 > > chrome.mediaGalleries.doSomething(function() { > > }); > > // user action = false, tab ID = undefined > > }); > > > > i.e. the chrome.mediaGalleries.doSomething call is executed with that user > > action / tab ID state because the event listener has it scoped, so if you read > > it from the generic SendRequest place it will be set. > > > > Sorry if I'm missing something. > > Ah I see what you mean. I'll try to narrow the scope down and see if we can get > it to a point where we can make it scoped. If you happen to know where that is, > that would be very helpful. So what I'd probably do is have that scoped thing in request_sender.h. Say, something like: class RequestSender { class ScopedTabID { explicit ScopedTabID(RequestSender* rs, int tab_id) : rs_(rs), tab_id_(tab_id) { old_ = rs->current_tab_id_; rs->current_tab_id_ = tab_id; } ~ScopedTabID() { DCHECK_EQ(tab_id_, rs->current_tab_id_); rs->current_tab_id_ = old_; } RequestSender* rs_; int tab_id_; } ... friend class ScopedTabID; int current_tab_id_; // start at -1 } then declare one of those inside the Dispatcher::DispatchEvent call? I think it's just event dispatching that you need? sorry rushing through this, hope that's right.
Ok, ScopedTabID it is. See patch set 4.
lgtm thanks so much for sticking with this so long. One more comment which hopefully can avoid a bunch of the plumbing that you had to do. https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... chrome/renderer/extensions/dispatcher.cc:620: port_to_tab_id_map_.erase(target_port); maybe you could also DCHECK(port_to_tab_id_map_.find(target_port) != port_to_tab_id_map_.end()) somewhere around here? https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... File chrome/renderer/extensions/request_sender.cc (right): https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... chrome/renderer/extensions/request_sender.cc:109: params.source_tab_id = source_tab_id; I was imagining the source_tab_id would be read from this->source_tab_id_ rather than needing to thread through messaging bindings, the custom bindings, and the JS? https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... File chrome/renderer/extensions/request_sender.h (right): https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... chrome/renderer/extensions/request_sender.h:90: int sender_tab_id() const { (see comment in request_sender.cc; does this need to be exposed at all?)
https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... chrome/renderer/extensions/dispatcher.cc:620: port_to_tab_id_map_.erase(target_port); On 2014/01/28 18:45:45, kalman wrote: > maybe you could also DCHECK(port_to_tab_id_map_.find(target_port) != > port_to_tab_id_map_.end()) somewhere around here? Done... and the DCHECK fails. It turns out in this case, the renderer is the source and the app is the target. Only the renderer process gets this call, not the app process . The fix was non-trivial, so please take a look at the patch set 4 -> 5 diff. Suggestions to do this better are of course welcome. https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... File chrome/renderer/extensions/request_sender.cc (right): https://codereview.chromium.org/145463002/diff/490001/chrome/renderer/extensi... chrome/renderer/extensions/request_sender.cc:109: params.source_tab_id = source_tab_id; On 2014/01/28 18:45:45, kalman wrote: > I was imagining the source_tab_id would be read from this->source_tab_id_ rather > than needing to thread through messaging bindings, the custom bindings, and the > JS? Ah, yes. That is much simpler. Done.
lgtm, and thanks for all of the cleanup along the way, it has really improved the code. https://codereview.chromium.org/145463002/diff/550001/chrome/renderer/extensi... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/145463002/diff/550001/chrome/renderer/extensi... chrome/renderer/extensions/dispatcher.cc:591: DCHECK_EQ(1, target_port_id % 2); // target renderer ports are assigned odd IDs
jww: ping https://codereview.chromium.org/145463002/diff/550001/chrome/renderer/extensi... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/145463002/diff/550001/chrome/renderer/extensi... chrome/renderer/extensions/dispatcher.cc:591: DCHECK_EQ(1, target_port_id % 2); On 2014/01/29 00:45:51, kalman wrote: > // target renderer ports are assigned odd IDs Done.
On 2014/01/29 00:53:22, Lei Zhang wrote: > jww: ping TBR=jww
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/145463002/580001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ bit was unchecked on CL. Ignoring.
Looks like we really do need security sign off. jww was somewhat positive here: https://code.google.com/p/chromium/issues/detail?id=333899#c19
Security folks: still waiting for a review. The only change in patch set 8 is in chrome/renderer/extensions/dispatcher.cc to fix the DCHECK failure.
ipc security lgtm. pod tab id
The CQ bit was checked by vandebo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/145463002/800001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by thestig@chromium.org
The CQ bit was unchecked by thestig@chromium.org
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
On 2014/02/02 05:52:34, I haz the power (commit-bot) wrote: > CQ bit was unchecked on CL. Ignoring. security lgtm
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/145463002/1290001
Message was sent while issue was closed.
Change committed as 249322 |
