Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(364)

Issue 2208483002: Fix extension bindings injection for iframes (reland) (Closed)

Created:
3 years, 4 months ago by asargent_no_longer_on_chrome
Modified:
3 years, 4 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 (reland) 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. This originally landed in codereview.chromium.org/2151693002/ but was reverted because of bug 630928 as well as the test failing under DrMemory (not with memory errors; just not succeeding which likely indicates some kind of race condition in the test). I've added a fix for bug 630928 but haven't been able to locally reproduce the test failure under DrMemory, so I've added some extra logging to the test to hopefully better understand what might be going wrong. Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation test fails again without any actual memory errors, please do not revert the entire CL (since it is an important security fix); instead just disable the test or add it to a suppression file so I can iterate on a fix. BUG=573131, 630928 Committed: https://crrev.com/79b64c3e741cc9c6afbb23885945831a45c6baa5 Cr-Commit-Position: refs/heads/master@{#409819}

Patch Set 1 : same as codereview.chromium.org/2151693002 #

Patch Set 2 : add fix and test for bug 630928, and logging to existing test #

Total comments: 9

Patch Set 3 : addressed review comments, fixed lint #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -17 lines) Patch
M chrome/browser/extensions/extension_bindings_apitest.cc View 2 chunks +61 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_messages_apitest.cc View 1 2 2 chunks +41 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/bindings/external_message_listener/background.js View 1 chunk +32 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/bindings/external_message_listener/manifest.json View 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/bindings/frames_before_navigation.html View 1 1 chunk +77 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/bindings/message_sender/background.js View 1 chunk +2 lines, -2 lines 0 comments Download
A chrome/test/data/extensions/api_test/bindings/message_sender/manifest.json View 1 chunk +13 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/bindings/message_sender/public.html View 1 chunk +1 line, -1 line 0 comments Download
A + chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/popup_opener.html View 1 2 1 chunk +4 lines, -9 lines 0 comments Download
M extensions/renderer/script_context.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M extensions/renderer/script_context.cc View 1 2 3 chunks +18 lines, -3 lines 0 comments Download
M extensions/renderer/script_context_set.cc View 1 2 1 chunk +8 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (17 generated)
asargent_no_longer_on_chrome
https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/script_context.cc File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/script_context.cc#newcode122 extensions/renderer/script_context.cc:122: : web_frame_->dataSource(); Note that there's a little duplication here ...
3 years, 4 months ago (2016-08-03 00:22:24 UTC) #8
Devlin
https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensions/extension_messages_apitest.cc File chrome/browser/extensions/extension_messages_apitest.cc (right): https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensions/extension_messages_apitest.cc#newcode1041 chrome/browser/extensions/extension_messages_apitest.cc:1041: for (int i = 0; i < browser()->tab_strip_model()->count(); i++) ...
3 years, 4 months ago (2016-08-03 16:16:49 UTC) #11
asargent_no_longer_on_chrome
PTAL https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensions/extension_messages_apitest.cc File chrome/browser/extensions/extension_messages_apitest.cc (right): https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensions/extension_messages_apitest.cc#newcode1041 chrome/browser/extensions/extension_messages_apitest.cc:1041: for (int i = 0; i < browser()->tab_strip_model()->count(); ...
3 years, 4 months ago (2016-08-04 05:47:18 UTC) #13
Devlin
lgtm
3 years, 4 months ago (2016-08-04 16:26:14 UTC) #18
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/2208483002/80001
3 years, 4 months ago (2016-08-04 17:12:24 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:80001)
3 years, 4 months ago (2016-08-04 17:17:32 UTC) #22
commit-bot: I haz the power
3 years, 4 months ago (2016-08-04 17:20:33 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/79b64c3e741cc9c6afbb23885945831a45c6baa5
Cr-Commit-Position: refs/heads/master@{#409819}

Powered by Google App Engine
This is Rietveld 408576698