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

Issue 1642283002: Deal with frame removal by content scripts (Closed)

Created:
4 years, 2 months ago by robwu
Modified:
4 years ago
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Deal with frame removal by content scripts Blink and the RenderFrame implementations are currently not prepared to deal with frame detachments in their callbacks. Consequently, extension code (content scripts, chrome.app.window.create) that run arbitrary code in the "document element created" and "document loaded" notifications may result in unexpected invalidation of memory, resulting in a UAF. This patch fixes the bug by moving all code that runs untrusted code from observers to dedicated callbacks, which are only run at a safe point. All document parsers in Blink have been modified to make sure that they still work even when the document creation is interrupted by frame removal. An extensive set of tests for all different kinds of documents, frame removal methods (e.g. synchronously / in mutation events / ...) and injection points (document start/end) have been added to avoid regressions. BUG=582008 Committed: https://crrev.com/43ea0649d4b70fdcf3e9fa5c03aee1bbba0b04bb Cr-Commit-Position: refs/heads/master@{#382162}

Patch Set 1 #

Patch Set 2 : Don't forget to check in test files... #

Patch Set 3 : Check for detaching in all Document types + tests #

Patch Set 4 : ImageDocument: Account for detach in DOM mutations #

Total comments: 4

Patch Set 5 : Run scripts at microtask checkpoint #

Patch Set 6 : Schedule microtask for document_end scripts #

Total comments: 2

Patch Set 7 : rebase, discard all changes except for Blink and tests #

Patch Set 8 : Add AfterDidCreateDocumentElement/AfterDidFinishDocumentLoad #

Total comments: 11

Patch Set 9 : Push down WeakPtr #

Total comments: 13

Patch Set 10 : Up to #50: ExtensionFrameHelper callbacks, comments #

Total comments: 13

Patch Set 11 : Use ScriptForbiddenScope in FrameLoader; Rename callbacks. #

Patch Set 12 : rebase #

Patch Set 13 : Add more runScriptsAtDocumentElementAvailable + comments #

Total comments: 19

Patch Set 14 : Improve tests #

Patch Set 15 : rebase #

Patch Set 16 : Use 1 test bucket, allow MojoBindingsController to run scripts #

Total comments: 4

Patch Set 17 : Last nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+791 lines, -28 lines) Patch
M chrome/browser/extensions/execute_script_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +78 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +18 lines, -0 lines 0 comments Download
M chrome/renderer/extensions/chrome_extensions_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/renderer/extensions/chrome_extensions_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +10 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/about_blank_frame.html View 1 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/audio.oga View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/audio.oga.mock-http-headers View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/audio_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/empty.html View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/empty_frame.html View 1 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/flag_document_end.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/image.png View 1 2 Binary file 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/image_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +33 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/no_url_frame.html View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/plain.txt View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/plugin_file.pdf View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/plugin_file.pdf.mock-http-headers View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/plugin_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +93 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/srcdoc_html_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/srcdoc_text_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/test.html View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/test.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +241 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/txt_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/executescript/destructive/video.ogv View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/video.ogv.mock-http-headers View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/video_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/xhtml_document.xhtml View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/xhtml_document.xhtml.mock-http-headers View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/xhtml_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/xml_document.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/executescript/destructive/xml_frame.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +9 lines, -1 line 0 comments Download
M content/renderer/mojo_bindings_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/mojo_bindings_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +5 lines, -5 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +32 lines, -0 lines 0 comments Download
M extensions/renderer/dispatcher.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M extensions/renderer/dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +18 lines, -0 lines 0 comments Download
M extensions/renderer/extension_frame_helper.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +27 lines, -0 lines 0 comments Download
M extensions/renderer/extension_frame_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +41 lines, -1 line 0 comments Download
M extensions/renderer/render_frame_observer_natives.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -5 lines 0 comments Download
M extensions/renderer/script_injection_manager.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +16 lines, -2 lines 0 comments Download
M extensions/shell/renderer/shell_content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M extensions/shell/renderer/shell_content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/ImageDocument.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +15 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/core/html/MediaDocument.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/html/PluginDocument.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +15 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/TextDocumentParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/EmptyClients.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +14 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoaderClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +14 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 101 (16 generated)
robwu
Devlin: extensions/renderer/script_injection_manager.cc nasko: content/renderer/render_frame_impl.cc Daniel: everything else (Blink, tests). I tracked down every call path ...
4 years, 1 month ago (2016-02-08 12:27:30 UTC) #2
Devlin
On 2016/02/08 12:27:30, robwu wrote: > Devlin: extensions/renderer/script_injection_manager.cc > > nasko: content/renderer/render_frame_impl.cc > > Daniel: ...
4 years, 1 month ago (2016-02-08 17:57:06 UTC) #3
nasko
Please add a bit more to the CL description. I had to read the code ...
4 years, 1 month ago (2016-02-08 18:12:53 UTC) #4
esprehn
Hmm, do we really need to tell extensions about the internal document types like ImageDocument? ...
4 years, 1 month ago (2016-02-09 00:37:09 UTC) #6
robwu
On 2016/02/08 18:12:53, nasko wrote: > Please add a bit more to the CL description. ...
4 years, 1 month ago (2016-02-09 00:53:46 UTC) #7
esprehn
I don't know how there would be a lot of DOM at the microtask if ...
4 years, 1 month ago (2016-02-09 01:11:16 UTC) #8
dcheng
On 2016/02/09 at 01:11:16, esprehn wrote: > I don't know how there would be a ...
4 years, 1 month ago (2016-02-09 01:15:31 UTC) #9
robwu
On 2016/02/09 01:11:16, esprehn wrote: > I don't know how there would be a lot ...
4 years, 1 month ago (2016-02-09 01:18:10 UTC) #10
esprehn
On 2016/02/09 at 01:18:10, rob wrote: > On 2016/02/09 01:11:16, esprehn wrote: > > I ...
4 years, 1 month ago (2016-02-09 01:21:17 UTC) #11
dcheng
On 2016/02/09 at 01:21:17, esprehn wrote: > On 2016/02/09 at 01:18:10, rob wrote: > > ...
4 years, 1 month ago (2016-02-09 01:24:42 UTC) #12
robwu
I have been experimenting with microtasks, and I see more disadvantages than advantages. Because the ...
4 years, 1 month ago (2016-02-10 00:51:27 UTC) #13
esprehn
I have to think about this more, in the meantime you should get kouhei to ...
4 years, 1 month ago (2016-02-10 01:08:57 UTC) #15
kouhei (in TOK)
On 2016/02/10 01:08:57, esprehn wrote: > I have to think about this more, in the ...
4 years, 1 month ago (2016-02-10 01:21:19 UTC) #16
kouhei (in TOK)
I don't think adding Microtask::performCheckpoint is a right solution. +haraken: would you confirm? https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp File ...
4 years, 1 month ago (2016-02-10 01:38:02 UTC) #18
haraken
+yhirano@ for Micotask::performCheckpoint
4 years, 1 month ago (2016-02-10 01:47:03 UTC) #20
yhirano
On 2016/02/10 01:47:03, haraken wrote: > +yhirano@ for Micotask::performCheckpoint I cannot see the crbug issue: ...
4 years, 1 month ago (2016-02-10 01:51:50 UTC) #21
robwu
https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp (right): https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp#newcode2296 third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:2296: if (m_parser->isStopped()) { On 2016/02/10 01:38:02, kouhei wrote: > ...
4 years, 1 month ago (2016-02-10 10:19:52 UTC) #22
yhirano
I made the CL protected and removed auto-cc.
4 years, 1 month ago (2016-02-10 18:50:42 UTC) #24
yhirano
I agree with https://codereview.chromium.org/1642283002/#msg13 and https://codereview.chromium.org/1642283002/#msg18. If we don't want to execute scripts in RenderFrameObserver::DidCreateDocumentElement, ...
4 years, 1 month ago (2016-02-10 19:53:46 UTC) #25
robwu
On 2016/02/10 19:53:46, yhirano wrote: > I agree with https://codereview.chromium.org/1642283002/#msg13 and > https://codereview.chromium.org/1642283002/#msg18. If we ...
4 years, 1 month ago (2016-02-10 20:33:08 UTC) #26
nasko
On 2016/02/10 20:33:08, robwu wrote: > On 2016/02/10 19:53:46, yhirano wrote: > > I agree ...
4 years, 1 month ago (2016-02-10 22:25:20 UTC) #27
robwu
On 2016/02/10 22:25:20, nasko wrote: > On 2016/02/10 20:33:08, robwu wrote: > > On 2016/02/10 ...
4 years, 1 month ago (2016-02-10 22:38:58 UTC) #28
nasko
On 2016/02/10 22:38:58, robwu wrote: > On 2016/02/10 22:25:20, nasko wrote: > > On 2016/02/10 ...
4 years, 1 month ago (2016-02-10 22:50:57 UTC) #29
esprehn
I'd prefer we didn't do that, I want this to be the only case we ...
4 years, 1 month ago (2016-02-10 23:06:11 UTC) #30
nasko
On 2016/02/10 23:06:11, esprehn wrote: > I'd prefer we didn't do that, I want this ...
4 years, 1 month ago (2016-02-10 23:25:38 UTC) #31
esprehn
On 2016/02/10 at 23:25:38, nasko wrote: > On 2016/02/10 23:06:11, esprehn wrote: > > I'd ...
4 years, 1 month ago (2016-02-10 23:53:03 UTC) #32
nasko
On 2016/02/10 23:53:03, esprehn wrote: > On 2016/02/10 at 23:25:38, nasko wrote: > > On ...
4 years, 1 month ago (2016-02-11 00:08:28 UTC) #33
robwu
I routed the script injection of all extension code via the ContentRendererClient (except for the ...
4 years, 1 month ago (2016-02-11 12:40:24 UTC) #34
nasko
https://codereview.chromium.org/1642283002/diff/140001/content/public/renderer/content_renderer_client.h File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1642283002/diff/140001/content/public/renderer/content_renderer_client.h#newcode311 content/public/renderer/content_renderer_client.h:311: virtual void AfterDidCreateDocumentElement( Why the "After" prefix? It is ...
4 years, 1 month ago (2016-02-11 16:49:01 UTC) #35
robwu
https://codereview.chromium.org/1642283002/diff/140001/content/public/renderer/content_renderer_client.h File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1642283002/diff/140001/content/public/renderer/content_renderer_client.h#newcode311 content/public/renderer/content_renderer_client.h:311: virtual void AfterDidCreateDocumentElement( On 2016/02/11 16:49:01, nasko wrote: > ...
4 years, 1 month ago (2016-02-11 17:51:09 UTC) #36
ncarter (slow)
https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/dispatcher.cc File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/dispatcher.cc#newcode507 extensions/renderer/dispatcher.cc:507: ExtensionFrameHelper::Get(render_frame.get()); Nasko pointed me at this CL to see ...
4 years, 1 month ago (2016-02-11 22:47:32 UTC) #38
robwu
https://codereview.chromium.org/1642283002/diff/140001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1642283002/diff/140001/content/renderer/render_frame_impl.cc#newcode3384 content/renderer/render_frame_impl.cc:3384: // Do not use |this| or |frame|! ContentClient may ...
4 years, 1 month ago (2016-02-11 23:36:51 UTC) #39
ncarter (slow)
lgtm https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/dispatcher.cc File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/dispatcher.cc#newcode507 extensions/renderer/dispatcher.cc:507: ExtensionFrameHelper::Get(render_frame.get()); On 2016/02/11 23:36:50, robwu wrote: > On ...
4 years, 1 month ago (2016-02-12 19:32:57 UTC) #40
robwu
Devlin: Could you review extensions/? content/ has been approved, so I don't expect major changes ...
4 years, 1 month ago (2016-02-12 19:51:43 UTC) #42
Devlin
haven't checked the tests yet. https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc#newcode1393 chrome/renderer/chrome_content_renderer_client.cc:1393: ->AfterDidCreateDocumentElement(render_frame); hmm... I'd say ...
4 years, 1 month ago (2016-02-12 22:21:56 UTC) #43
robwu
https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc#newcode1393 chrome/renderer/chrome_content_renderer_client.cc:1393: ->AfterDidCreateDocumentElement(render_frame); On 2016/02/12 22:21:56, Devlin wrote: > hmm... I'd ...
4 years, 1 month ago (2016-02-12 22:37:13 UTC) #44
Devlin
https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc#newcode512 extensions/renderer/dispatcher.cc:512: script_injection_manager_->AfterDidCreateDocumentElement(render_frame); On 2016/02/12 22:37:13, robwu wrote: > On 2016/02/12 ...
4 years, 1 month ago (2016-02-12 22:51:01 UTC) #45
esprehn
On 2016/02/12 at 22:37:13, rob wrote: > ... > > Blink has ScriptForbiddenScope, but it's ...
4 years, 1 month ago (2016-02-12 22:52:42 UTC) #46
dcheng
On 2016/02/12 at 22:52:42, esprehn wrote: > On 2016/02/12 at 22:37:13, rob wrote: > > ...
4 years, 1 month ago (2016-02-12 22:58:00 UTC) #47
esprehn
On 2016/02/12 at 22:58:00, dcheng wrote: > On 2016/02/12 at 22:52:42, esprehn wrote: > > ...
4 years, 1 month ago (2016-02-12 23:14:13 UTC) #48
robwu
https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc#newcode512 extensions/renderer/dispatcher.cc:512: script_injection_manager_->AfterDidCreateDocumentElement(render_frame); On 2016/02/12 22:51:01, Devlin wrote: > On 2016/02/12 ...
4 years, 1 month ago (2016-02-12 23:24:49 UTC) #49
Devlin
https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/dispatcher.cc#newcode512 extensions/renderer/dispatcher.cc:512: script_injection_manager_->AfterDidCreateDocumentElement(render_frame); On 2016/02/12 23:24:49, robwu wrote: > On 2016/02/12 ...
4 years, 1 month ago (2016-02-12 23:42:24 UTC) #50
robwu
I've added some comments to content/ and addressed all review points in patchset 10.
4 years, 1 month ago (2016-02-13 00:28:25 UTC) #51
ncarter (slow)
https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome_content_renderer_client.cc#newcode1393 chrome/renderer/chrome_content_renderer_client.cc:1393: ->AfterDidCreateDocumentElement(render_frame); On 2016/02/12 22:37:13, robwu wrote: > On 2016/02/12 ...
4 years, 1 month ago (2016-02-16 18:22:32 UTC) #52
Devlin
Still waiting on the blink folks' lg to look at the tests in depth, but ...
4 years, 1 month ago (2016-02-16 18:37:37 UTC) #53
esprehn
You need a better change description, you need to explain what you're doing and why. ...
4 years, 1 month ago (2016-02-17 01:43:47 UTC) #54
robwu
On 2016/02/17 01:43:47, esprehn wrote: > You need a better change description, you need to ...
4 years, 1 month ago (2016-02-17 23:49:05 UTC) #55
esprehn
On 2016/02/17 at 23:49:05, rob wrote: > On 2016/02/17 01:43:47, esprehn wrote: > > You ...
4 years, 1 month ago (2016-02-18 00:10:55 UTC) #56
esprehn
sigh, not lgtm, forgot I can't say it :)
4 years, 1 month ago (2016-02-18 00:11:17 UTC) #57
robwu
On 2016/02/18 00:10:55, esprehn wrote: > On 2016/02/17 at 23:49:05, rob wrote: > > On ...
4 years, 1 month ago (2016-02-18 01:20:36 UTC) #59
dcheng
On 2016/02/18 at 01:20:36, rob wrote: > On 2016/02/18 00:10:55, esprehn wrote: > > On ...
4 years, 1 month ago (2016-02-18 01:35:45 UTC) #60
robwu
This CL is blocked on replies from esprehn and dcheng. esprehn: In comment #55, I ...
4 years, 1 month ago (2016-02-19 22:55:22 UTC) #61
esprehn
I'm loooking at the code for DidFinishDocumentLoad for workers, it doesn't seem to run any ...
4 years, 1 month ago (2016-02-19 23:02:47 UTC) #62
dcheng
On 2016/02/19 at 22:55:22, rob wrote: > This CL is blocked on replies from esprehn ...
4 years, 1 month ago (2016-02-19 23:06:33 UTC) #63
robwu
On 2016/02/19 23:02:47, esprehn wrote: > I'm loooking at the code for DidFinishDocumentLoad for workers, ...
4 years, 1 month ago (2016-02-19 23:32:52 UTC) #64
robwu
On 2016/02/19 23:06:33, dcheng wrote: > > dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously run a > callback ...
4 years, 1 month ago (2016-02-19 23:41:00 UTC) #65
dcheng
On 2016/02/19 at 23:41:00, rob wrote: > On 2016/02/19 23:06:33, dcheng wrote: > > > ...
4 years, 1 month ago (2016-02-19 23:54:25 UTC) #67
Ken Rockot(use gerrit already)
On 2016/02/19 at 23:54:25, dcheng wrote: > On 2016/02/19 at 23:41:00, rob wrote: > > ...
4 years, 1 month ago (2016-02-21 16:03:48 UTC) #68
robwu
Thanks for your confirmation rockot@. esprehn: I've added ScriptForbiddenScope in FrameLoader (2x) and new Blink ...
4 years, 1 month ago (2016-02-22 00:15:02 UTC) #69
dcheng
https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc#newcode158 extensions/renderer/extension_frame_helper.cc:158: document_element_created_callbacks_.push_back(callback); On 2016/02/22 at 00:15:02, robwu wrote: > On ...
4 years, 1 month ago (2016-02-22 00:28:34 UTC) #70
robwu
https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc#newcode158 extensions/renderer/extension_frame_helper.cc:158: document_element_created_callbacks_.push_back(callback); On 2016/02/22 00:28:34, dcheng wrote: > On 2016/02/22 ...
4 years, 1 month ago (2016-02-22 00:52:31 UTC) #71
dcheng
On 2016/02/22 at 00:52:31, rob wrote: > https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc > File extensions/renderer/extension_frame_helper.cc (right): > > https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/extension_frame_helper.cc#newcode158 ...
4 years, 1 month ago (2016-02-22 17:51:32 UTC) #72
robwu
Ping esprehn. I was hoping to get this in the first release of M-49, but ...
4 years, 1 month ago (2016-02-23 16:17:58 UTC) #73
robwu
Ping esprehn.
4 years, 1 month ago (2016-02-26 20:49:29 UTC) #74
kouhei (in TOK)
re: parser changes, have we agreed that we keep the design that dispatchDocumentElementAvailable() may invalidate ...
4 years, 1 month ago (2016-02-26 21:48:37 UTC) #75
robwu
On 2016/02/26 21:48:37, kouhei wrote: > re: parser changes, have we agreed that we keep ...
4 years, 1 month ago (2016-02-26 21:59:24 UTC) #76
kouhei (in TOK)
On 2016/02/26 21:59:24, robwu wrote: > On 2016/02/26 21:48:37, kouhei wrote: > > re: parser ...
4 years, 1 month ago (2016-02-26 23:12:43 UTC) #77
esprehn
On 2016/02/26 at 21:59:24, rob wrote: > On 2016/02/26 21:48:37, kouhei wrote: > > re: ...
4 years, 1 month ago (2016-02-26 23:18:24 UTC) #78
robwu
On 2016/02/26 23:18:24, esprehn wrote: > On 2016/02/26 at 21:59:24, rob wrote: > > On ...
4 years, 1 month ago (2016-02-26 23:34:56 UTC) #79
robwu
Esprehn: Please take a look. Now the dispatchDocumentElementAvailable notification does not run any scripts at ...
4 years ago (2016-03-05 01:50:58 UTC) #80
robwu
Ping for review. @esprehn @kouhei
4 years ago (2016-03-09 16:54:51 UTC) #81
robwu
Ping again... Come on, this is a security bug! Why is there no response for ...
4 years ago (2016-03-14 23:07:49 UTC) #82
esprehn
lgtm, looks legit.
4 years ago (2016-03-15 00:06:52 UTC) #83
kouhei (in TOK)
lgtm for html/parser
4 years ago (2016-03-15 00:19:58 UTC) #84
Devlin
I took another quick look, and will look more in-depth tomorrow. https://codereview.chromium.org/1642283002/diff/240001/chrome/browser/extensions/execute_script_apitest.cc File chrome/browser/extensions/execute_script_apitest.cc (right): ...
4 years ago (2016-03-15 00:53:32 UTC) #85
Devlin
https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js File chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js (right): https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js#newcode22 chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:22: if (event.data === 'toBeRemoved') Do we expect any other ...
4 years ago (2016-03-15 16:34:35 UTC) #86
robwu
https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js File chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js (right): https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js#newcode22 chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:22: if (event.data === 'toBeRemoved') On 2016/03/15 16:34:34, Devlin wrote: ...
4 years ago (2016-03-16 22:03:32 UTC) #88
Devlin
Looks like this breaks webkit tests. :/ https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/test.js File chrome/test/data/extensions/api_test/executescript/destructive/test.js (right): https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/extensions/api_test/executescript/destructive/test.js#newcode158 chrome/test/data/extensions/api_test/executescript/destructive/test.js:158: var kTestsPerInstance ...
4 years ago (2016-03-17 16:59:02 UTC) #89
robwu
I've replaced DidCreateDocumentElement with RunScriptsAtDocumentStart in MojoBindingsController to fix the crash in the WebKit tests ...
4 years ago (2016-03-17 22:20:40 UTC) #90
Devlin
awesome, extensions lgtm. https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js File chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js (right): https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js#newcode78 chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:78: if (performance.timing.loadEventEnd > 0) nit: while ...
4 years ago (2016-03-18 16:46:10 UTC) #91
robwu
sky: Please rs lg chrome/renderer/chrome_content_renderer_client.{h,cc} https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js File chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js (right): https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js#newcode78 chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:78: if (performance.timing.loadEventEnd > 0) ...
4 years ago (2016-03-18 19:25:38 UTC) #93
sky
LGTM
4 years ago (2016-03-18 20:11:40 UTC) #94
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1642283002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1642283002/320001
4 years ago (2016-03-18 21:46:03 UTC) #97
commit-bot: I haz the power
Committed patchset #17 (id:320001)
4 years ago (2016-03-19 01:05:12 UTC) #99
commit-bot: I haz the power
4 years ago (2016-03-19 01:06:26 UTC) #101
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/43ea0649d4b70fdcf3e9fa5c03aee1bbba0b04bb
Cr-Commit-Position: refs/heads/master@{#382162}

Powered by Google App Engine
This is Rietveld 408576698