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

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

Created:
4 years ago by robwu
Modified:
3 years, 11 months 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 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 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 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 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 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 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 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 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 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 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 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 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 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 ago (2016-02-10 01:38:02 UTC) #18
haraken
+yhirano@ for Micotask::performCheckpoint
4 years 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 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 ago (2016-02-10 10:19:52 UTC) #22
yhirano
I made the CL protected and removed auto-cc.
4 years 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 ago (2016-02-18 00:10:55 UTC) #56
esprehn
sigh, not lgtm, forgot I can't say it :)
4 years 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 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 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 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 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 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 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 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 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 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 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 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 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 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 ...
3 years, 12 months ago (2016-02-23 16:17:58 UTC) #73
robwu
Ping esprehn.
3 years, 12 months 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 ...
3 years, 12 months 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 ...
3 years, 12 months 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 ...
3 years, 12 months 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: ...
3 years, 12 months 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 ...
3 years, 12 months 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 ...
3 years, 11 months ago (2016-03-05 01:50:58 UTC) #80
robwu
Ping for review. @esprehn @kouhei
3 years, 11 months 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 ...
3 years, 11 months ago (2016-03-14 23:07:49 UTC) #82
esprehn
lgtm, looks legit.
3 years, 11 months ago (2016-03-15 00:06:52 UTC) #83
kouhei (in TOK)
lgtm for html/parser
3 years, 11 months 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): ...
3 years, 11 months 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 ...
3 years, 11 months 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: ...
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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) ...
3 years, 11 months ago (2016-03-18 19:25:38 UTC) #93
sky
LGTM
3 years, 11 months 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
3 years, 11 months ago (2016-03-18 21:46:03 UTC) #97
commit-bot: I haz the power
Committed patchset #17 (id:320001)
3 years, 11 months ago (2016-03-19 01:05:12 UTC) #99
commit-bot: I haz the power
3 years, 11 months 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