Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400523002:
Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path. (Closed)
|
Created:
4 years, 2 months ago by Łukasz Anforowicz Modified:
4 years, 2 months ago Reviewers:
lazyboy CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIsolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path.
BUG=652708
Committed: https://crrev.com/2a130dcac2715c1ebf7e2666355e007bd71f3b44
Cr-Commit-Position: refs/heads/master@{#423360}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Self-review (removed unneeded includes + tweaked how test GURL is built). #
Total comments: 2
Patch Set 3 : Addressed CR feedback from lazyboy@ (removed one more unneeded #include). #
Messages
Total messages: 17 (11 generated)
The CQ bit was checked by lukasza@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...
lukasza@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy@, could you please take a look? https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (left): https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:271: if (effective_site_url.SchemeIs(extensions::kExtensionScheme)) { The check for extensions scheme is not necessary here - it is sufficient to see if GetExtensionOrAppByURL returns an extension. Furthermore, GetExtensionOrAppByURL can return an extension for some web URLs (in case of Chrome Web Store gallery URLs + in case of urls handled by "url handlers" registered in an app manifest [1,2]). Because of the above, it seems best to just remove the outer-most "if" statement here. [1] https://developer.chrome.com/apps/manifest/url_handlers [2] https://chromium.googlesource.com/chromium/src/+/3c19074450571b0fdd16063353e7... https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:276: if (extension) { Below I am replicating - if (extension && !extension->is_hosted_app() && - !extension->is_platform_app()) { return true; - } from the old code, but 1) making an exception for Chrome Web Store (to always isolate) and 2) splitting the single, complex decision into multiple separate, simple decisions (one for web store, then for the hosted apps exception, then returning true for all other extensions). https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:290: if (extension->is_hosted_app() || extension->is_platform_app()) Probably best to preserve the old behavior above (which is what the CL does above), but I wondered whether we could simply use |if (extension->is_app()) here? Definition this would go into is quoted below - as you can see it also covers "legacy packaged apps" (whatever they are :-). extensions/common/manifest.h: ... bool is_app() const { return is_legacy_packaged_app() || is_hosted_app() || is_platform_app(); } https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:379: EXPECT_NE(old_process_host, new_process_host); The test expectation above would fail before the changes in the other file touched by this CL, but it would fail because there would be a renderer kill before we get here :-) [136787:136787:1005/152500:ERROR:bad_message.cc(23)] Terminating renderer for bad IPC message, reason 1 [136787:136787:1005/152500:WARNING:sad_tab.cc(205)] Tab Killed: http://foo.com:54311/ ../../chrome/browser/extensions/process_management_browsertest.cc:371: Failure Value of: web_contents->GetLastCommittedURL() Actual: http://foo.com:54311/form.html Expected: cws_web_url Which is: http://chrome.webstore.test.com:54311/title1.html ../../chrome/browser/extensions/process_management_browsertest.cc:378: Failure Value of: extensions::ProcessMap::Get(profile())->Contains( extensions::kWebStoreAppId, new_process_host->GetID()) Actual: false Expected: true ../../chrome/browser/extensions/process_management_browsertest.cc:382: Failure Expected: (old_process_host) != (new_process_host), actual: 0x619000166c80 vs 0x619000166c80 [136787:136787:1005/152501:WARNING:url_request_context_getter.cc(43)] URLRequestContextGetter leaking due to no owning thread.
The CQ bit was checked by lukasza@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...
lgtm. https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:290: if (extension->is_hosted_app() || extension->is_platform_app()) On 2016/10/05 22:34:56, Łukasz Anforowicz wrote: > Probably best to preserve the old behavior above (which is what the CL does > above), but I wondered whether we could simply use |if (extension->is_app()) > here? Definition this would go into is quoted below - as you can see it also > covers "legacy packaged apps" (whatever they are :-). > > extensions/common/manifest.h: > ... > bool is_app() const { > return is_legacy_packaged_app() || is_hosted_app() || is_platform_app(); > } Given that we have separate plans for dealing with hosted apps and platform apps (#a, #b above), I think it's better to keep it as is. https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2400523002/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:379: EXPECT_NE(old_process_host, new_process_host); On 2016/10/05 22:34:56, Łukasz Anforowicz wrote: > The test expectation above would fail before the changes in the other file > touched by this CL, but it would fail because there would be a renderer kill > before we get here :-) > > [136787:136787:1005/152500:ERROR:bad_message.cc(23)] Terminating renderer for > bad IPC message, reason 1 > [136787:136787:1005/152500:WARNING:sad_tab.cc(205)] Tab Killed: > http://foo.com:54311/ > ../../chrome/browser/extensions/process_management_browsertest.cc:371: Failure > Value of: web_contents->GetLastCommittedURL() > Actual: http://foo.com:54311/form.html > Expected: cws_web_url > Which is: http://chrome.webstore.test.com:54311/title1.html > ../../chrome/browser/extensions/process_management_browsertest.cc:378: Failure > Value of: extensions::ProcessMap::Get(profile())->Contains( > extensions::kWebStoreAppId, new_process_host->GetID()) > Actual: false > Expected: true > ../../chrome/browser/extensions/process_management_browsertest.cc:382: Failure > Expected: (old_process_host) != (new_process_host), actual: 0x619000166c80 vs > 0x619000166c80 > [136787:136787:1005/152501:WARNING:url_request_context_getter.cc(43)] > URLRequestContextGetter leaking due to no owning thread. > > Acknowledged. https://codereview.chromium.org/2400523002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2400523002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:27: #include "extensions/browser/extension_registry.h" Not needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2400523002/#ps40001 (title: "Addressed CR feedback from lazyboy@ (removed one more unneeded #include).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. https://codereview.chromium.org/2400523002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2400523002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:27: #include "extensions/browser/extension_registry.h" On 2016/10/05 23:20:02, lazyboy wrote: > Not needed? Oh, right. Not sure how I missed that one :-) Done.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path. BUG=652708 ========== to ========== Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path. BUG=652708 Committed: https://crrev.com/2a130dcac2715c1ebf7e2666355e007bd71f3b44 Cr-Commit-Position: refs/heads/master@{#423360} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2a130dcac2715c1ebf7e2666355e007bd71f3b44 Cr-Commit-Position: refs/heads/master@{#423360}
Message was sent while issue was closed.
Description was changed from ========== Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path. BUG=652708 Committed: https://crrev.com/2a130dcac2715c1ebf7e2666355e007bd71f3b44 Cr-Commit-Position: refs/heads/master@{#423360} ========== to ========== Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path. BUG=652708 Committed: https://crrev.com/2a130dcac2715c1ebf7e2666355e007bd71f3b44 Cr-Commit-Position: refs/heads/master@{#423360} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
