Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9)

Issue 2151693002: Fix extension bindings injection for iframes (Closed)

Created:
4 years, 5 months ago by asargent_no_longer_on_chrome
Modified:
4 years, 5 months ago
Reviewers:
Devlin
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix extension bindings injection for iframes For iframes, we don't want to use the source url for determining the associated extension because it starts out with an about:blank context that is scriptable by its parent. BUG=573131 Committed: https://crrev.com/91f655b19888da3f86b57ad8c548da93e7b9aba4 Cr-Commit-Position: refs/heads/master@{#407214}

Patch Set 1 #

Total comments: 11

Patch Set 2 : use security origin, augmented tests, fix nits #

Total comments: 16

Patch Set 3 : more review fixes #

Total comments: 1

Patch Set 4 : hacker_case -> camelCase #

Total comments: 6

Patch Set 5 : more fixes from review feedback #

Messages

Total messages: 24 (9 generated)
asargent_no_longer_on_chrome
4 years, 5 months ago (2016-07-13 23:09:20 UTC) #2
Devlin
https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc File extensions/renderer/script_context_set.cc (right): https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc#newcode151 extensions/renderer/script_context_set.cc:151: GURL frame_url = frame->parent() Hmm... so what happens if ...
4 years, 5 months ago (2016-07-13 23:19:09 UTC) #3
asargent_no_longer_on_chrome
https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc File extensions/renderer/script_context_set.cc (right): https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc#newcode151 extensions/renderer/script_context_set.cc:151: GURL frame_url = frame->parent() On 2016/07/13 23:19:09, Devlin wrote: ...
4 years, 5 months ago (2016-07-14 16:38:31 UTC) #4
Devlin
https://codereview.chromium.org/2151693002/diff/1/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js File chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js (right): https://codereview.chromium.org/2151693002/diff/1/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js#newcode9 chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js:9: if (msg == "from_sender") { prefer single quotes in ...
4 years, 5 months ago (2016-07-14 16:46:36 UTC) #5
asargent_no_longer_on_chrome
https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc File extensions/renderer/script_context_set.cc (right): https://codereview.chromium.org/2151693002/diff/1/extensions/renderer/script_context_set.cc#newcode151 extensions/renderer/script_context_set.cc:151: GURL frame_url = frame->parent() On 2016/07/14 16:46:35, Devlin wrote: ...
4 years, 5 months ago (2016-07-14 21:38:07 UTC) #6
asargent_no_longer_on_chrome
ptal https://codereview.chromium.org/2151693002/diff/1/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js File chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js (right): https://codereview.chromium.org/2151693002/diff/1/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js#newcode9 chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js:9: if (msg == "from_sender") { On 2016/07/14 16:46:35, ...
4 years, 5 months ago (2016-07-20 21:36:47 UTC) #8
Devlin
Nice! This approach looks much better. :) https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc File chrome/browser/extensions/extension_bindings_apitest.cc (right): https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc#newcode213 chrome/browser/extensions/extension_bindings_apitest.cc:213: // chrome-extenison:// ...
4 years, 5 months ago (2016-07-21 00:44:54 UTC) #9
asargent_no_longer_on_chrome
https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc File chrome/browser/extensions/extension_bindings_apitest.cc (right): https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc#newcode213 chrome/browser/extensions/extension_bindings_apitest.cc:213: // chrome-extenison:// urls, both web_accessible and non-existing pages, don't ...
4 years, 5 months ago (2016-07-21 18:12:22 UTC) #13
asargent_no_longer_on_chrome
https://codereview.chromium.org/2151693002/diff/80001/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js File chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js (right): https://codereview.chromium.org/2151693002/diff/80001/chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js#newcode8 chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js:8: var received_real_sender_message = false; gah, just realized I named ...
4 years, 5 months ago (2016-07-21 18:16:33 UTC) #14
Devlin
lgtm with nits https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc File chrome/browser/extensions/extension_bindings_apitest.cc (right): https://codereview.chromium.org/2151693002/diff/40001/chrome/browser/extensions/extension_bindings_apitest.cc#newcode222 chrome/browser/extensions/extension_bindings_apitest.cc:222: base::CommandLine::ForCurrentProcess()->AppendSwitch( On 2016/07/21 18:12:22, Antony Sargent ...
4 years, 5 months ago (2016-07-21 22:20:34 UTC) #15
asargent_no_longer_on_chrome
> I have two concerns: > - If we ever change chrome to cache this ...
4 years, 5 months ago (2016-07-22 17:36:51 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2151693002/120001
4 years, 5 months ago (2016-07-22 17:40:15 UTC) #20
commit-bot: I haz the power
Committed patchset #5 (id:120001)
4 years, 5 months ago (2016-07-22 18:39:32 UTC) #21
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/91f655b19888da3f86b57ad8c548da93e7b9aba4 Cr-Commit-Position: refs/heads/master@{#407214}
4 years, 5 months ago (2016-07-22 18:41:42 UTC) #23
benwells
4 years, 4 months ago (2016-07-28 06:49:28 UTC) #24
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.chromium.org/2191793002/ by benwells@chromium.org.

The reason for reverting is: This Cl caused a test failure under DrMemory.

First build with failing test:
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%2...

Sample failure output: FramesExtensionBindingsApiTest.FramesBeforeNavigation:

[1420:4432:0722/173632:WARNING:chrome_browser_main_win.cc(419)] Command line too
long for RegisterApplicationRestart

[1420:4432:0722/174006:INFO:CONSOLE(0)] "Denying load of
chrome-extension://ficgdghpakbhhkmdjamiedmcoobamkoo/nonexistent.html. Resources
must be listed in the web_accessible_resources manifest key in order to be
loaded by pages outside the extension.", source: about:blank (0)

[1420:4432:0722/174015:INFO:CONSOLE(24)] "caught exception: SecurityError:
Blocked a frame with origin "http://127.0.0.1:50285" from accessing a
cross-origin frame.", source:
http://127.0.0.1:50285/extensions/api_test/bindings/frames_before_navigation....
(24)

[1420:4432:0722/174018:INFO:CONSOLE(0)] "Denying load of
chrome-extension://ficgdghpakbhhkmdjamiedmcoobamkoo/nonexistent.html. Resources
must be listed in the web_accessible_resources manifest key in order to be
loaded by pages outside the extension.", source: about:blank (0)

[1420:4988:0722/174053:WARNING:embedded_test_server.cc(193)] Request not
handled. Returning 404: /favicon.ico

c:\b\build\slave\drm-cr\build\src\chrome\browser\extensions\extension_bindings_apitest.cc(257):
error: Value of: page_success

Actual: false

Expected: true.

Powered by Google App Engine
This is Rietveld 408576698