Deal with frame removal by content scripts

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}

[robwu, 2016-02-08 12:27:29]

rob@robwu.nl changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org, rdevlin.cronin@chromium.org

[robwu, 2016-02-08 12:27:30]

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 up to a
safe point from FrameLoader::dispatchDocumentElementAvailable, and added checks
where necessary.

[Devlin, 2016-02-08 17:57:06]

On 2016/02/08 12:27:30, robwu wrote:
> 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 up to a
> safe point from FrameLoader::dispatchDocumentElementAvailable, and added
checks
> where necessary.

I'm gonna wait for Nasko's/Daniel's input before I do a really in depth review
(since most changes are in blink), but in principle looks okay.

[nasko, 2016-02-08 18:12:53]

Please add a bit more to the CL description. I had to read the code to
understand what this CL is aiming to achieve. Ideally, the description should
deliver the message and the code should be inspected to verify the expected
change.

https://codereview.chromium.org/1642283002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1642283002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3354: if
(observers_.might_have_observers()) {
Yuck :(. Why is this the only method that needs this functionality? Shouldn't
most observer interfaces that notify extensions be fixed in the same way?

[esprehn, 2016-02-09 00:37:09]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-02-09 00:37:09]

Hmm, do we really need to tell extensions about the internal document types like
ImageDocument? I guess there's some extensions that inject into the page loaded
when you load a .png directly?

https://codereview.chromium.org/1642283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1642283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:182:
m_webFrame->client()->didCreateDocumentElement(m_webFrame);
Woah what, there's script down here??

Can we instead schedule a micro task and yield the parser instead? :)

[robwu, 2016-02-09 00:53:46]

On 2016/02/08 18:12:53, nasko wrote:
> Please add a bit more to the CL description.

All details are in the linked bug report.

I'll address your other comment tomorrow.

On 2016/02/09 00:37:09, esprehn wrote:
> Hmm, do we really need to tell extensions about the internal document types
like
> ImageDocument? I guess there's some extensions that inject into the page
loaded
> when you load a .png directly?

Yes, it is needed. ImageDocument is just an implementation detail, web/extension
devs just see a HTML document with an image in it. I have an extension that
detects <embed> tags in (plugin) documents and then does something special with
it. And when I search for "image" in the Chrome Web Store, I see many results
that should work with ImageDocuments.

https://codereview.chromium.org/1642283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1642283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:182:
m_webFrame->client()->didCreateDocumentElement(m_webFrame);
On 2016/02/09 00:37:09, esprehn wrote:
> Woah what, there's script down here??
> 
> Can we instead schedule a micro task and yield the parser instead? :)

That could mean that a lot of DOM is generated, right? If so, then no.
The contract for when document_start scripts run is quite clear in this regard:
https://developer.chrome.com/extensions/content_scripts#run_at

On the other hand, if the microtask runs quite soon (like, right after the
document looks like <html><head></head><body></body></html>), then I think that
breaking the contact is not going to be that bad for extension compatibility.

[esprehn, 2016-02-09 01:11:16]

I don't know how there would be a lot of DOM at the microtask if we yield the
parser, the only case is the image/plugin documents which sync build the DOM
inside them. For those we could call performMicrotaskCheckpoint() directly,
which is super explicit as a "SCRIPT WILL RUN" signal.

The RenderFrameImpl change is scary to me, what other hidden script runs down
there? Ideally we should be putting WebScriptForbiddenScope all over until we
understand exactly what happens in the system.

[dcheng, 2016-02-09 01:15:31]

On 2016/02/09 at 01:11:16, esprehn wrote:
> I don't know how there would be a lot of DOM at the microtask if we yield the
parser, the only case is the image/plugin documents which sync build the DOM
inside them. For those we could call performMicrotaskCheckpoint() directly,
which is super explicit as a "SCRIPT WILL RUN" signal.
> 
> The RenderFrameImpl change is scary to me, what other hidden script runs down
there? Ideally we should be putting WebScriptForbiddenScope all over until we
understand exactly what happens in the system.

Not directly related, but https://crbug.com/555773 is where I started tracking
'embedder runs way too much script in callbacks' issues.

[robwu, 2016-02-09 01:18:10]

On 2016/02/09 01:11:16, esprehn wrote:
> I don't know how there would be a lot of DOM at the microtask if we yield the
> parser, the only case is the image/plugin documents which sync build the DOM
> inside them. For those we could call performMicrotaskCheckpoint() directly,
> which is super explicit as a "SCRIPT WILL RUN" signal.

This has potential. What about the most common case (HTML documents)? Do you
know whether the parser will yield soon enough?
(I can try your idea and see if things break, but if you know the answer already
;) )

> The RenderFrameImpl change is scary to me, what other hidden script runs down
> there? Ideally we should be putting WebScriptForbiddenScope all over until we
> understand exactly what happens in the system.

... that would certainly break extensions. The analysis was easier than
expected, see my reply to Nasko.

https://codereview.chromium.org/1642283002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1642283002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3354: if
(observers_.might_have_observers()) {
On 2016/02/08 18:12:53, nasko wrote:
> Yuck :(. Why is this the only method that needs this functionality? Shouldn't
> most observer interfaces that notify extensions be fixed in the same way?

This is needed because the observers expect the subject (RenderFrame) to be
valid, so that e.g. RenderFrameObserver::render_frame() returns a valid pointer.
But because content script execute synchronously, this assumption does not hold,
so we have to tread carefully, and check whether the RenderFrame is still valid.

I checked every file in extensions/renderer that includes
render_frame_observer.h, and found a few occurrences:
- extensions_render_frame_observer.h - safe (no untrusted calls)
- extension_frame_helper.h - safe (no untrusted calls in observer notifications)
- render_frame_observer_natives.cc - DidFailProvisionalLoad should be addressed
(that is for another CL, because it is unrelated to this specific issue and I
found another bug - crbug.com/585268).
- script_injection_manager.cc - all fixed by this CL.
- app_window_custom_bindings.cc - fixed by this CL.

[esprehn, 2016-02-09 01:21:17]

On 2016/02/09 at 01:18:10, rob wrote:
> On 2016/02/09 01:11:16, esprehn wrote:
> > I don't know how there would be a lot of DOM at the microtask if we yield
the
> > parser, the only case is the image/plugin documents which sync build the DOM
> > inside them. For those we could call performMicrotaskCheckpoint() directly,
> > which is super explicit as a "SCRIPT WILL RUN" signal.
> 
> This has potential. What about the most common case (HTML documents)? Do you
know whether the parser will yield soon enough?

I'm saying we should _force_ the parser to yield. :)


> [...] 
> 
> This is needed because the observers expect the subject (RenderFrame) to be
valid, so that e.g. RenderFrameObserver::render_frame() returns a valid pointer.
But because content script execute synchronously, this assumption does not hold,
so we have to tread carefully, and check whether the RenderFrame is still valid.

Yup, the question is why not everywhere else.

> 
> I checked every file in extensions/renderer that includes
render_frame_observer.h, and found a few occurrences:
> - extensions_render_frame_observer.h - safe (no untrusted calls)
> - extension_frame_helper.h - safe (no untrusted calls in observer
notifications)
> - render_frame_observer_natives.cc - DidFailProvisionalLoad should be
addressed (that is for another CL, because it is unrelated to this specific
issue and I found another bug - crbug.com/585268).
> - script_injection_manager.cc - all fixed by this CL.
> - app_window_custom_bindings.cc - fixed by this CL.

You should land a WebScriptForbidenScope to anywhere you think is "safe" to
prevent any future script from being added. It's the only way to be sure.

[dcheng, 2016-02-09 01:24:42]

On 2016/02/09 at 01:21:17, esprehn wrote:
> On 2016/02/09 at 01:18:10, rob wrote:
> > On 2016/02/09 01:11:16, esprehn wrote:
> > > I don't know how there would be a lot of DOM at the microtask if we yield
the
> > > parser, the only case is the image/plugin documents which sync build the
DOM
> > > inside them. For those we could call performMicrotaskCheckpoint()
directly,
> > > which is super explicit as a "SCRIPT WILL RUN" signal.
> > 
> > This has potential. What about the most common case (HTML documents)? Do you
know whether the parser will yield soon enough?
> 
> I'm saying we should _force_ the parser to yield. :)
> 
> 
> > [...] 
> > 
> > This is needed because the observers expect the subject (RenderFrame) to be
valid, so that e.g. RenderFrameObserver::render_frame() returns a valid pointer.
But because content script execute synchronously, this assumption does not hold,
so we have to tread carefully, and check whether the RenderFrame is still valid.
> 
> Yup, the question is why not everywhere else.

In general, we consider it an anti-pattern for observers to mutate the thing in
a way that will break other observers =)

We've fixed some bugs in the past around things like the NTP synchronously
navigating in WebFrameClient callbacks, causing the
WebLocalFrame/RenderFrameImpl to be blown away before other observers run.

> 
> > 
> > I checked every file in extensions/renderer that includes
render_frame_observer.h, and found a few occurrences:
> > - extensions_render_frame_observer.h - safe (no untrusted calls)
> > - extension_frame_helper.h - safe (no untrusted calls in observer
notifications)
> > - render_frame_observer_natives.cc - DidFailProvisionalLoad should be
addressed (that is for another CL, because it is unrelated to this specific
issue and I found another bug - crbug.com/585268).
> > - script_injection_manager.cc - all fixed by this CL.
> > - app_window_custom_bindings.cc - fixed by this CL.
> 
> You should land a WebScriptForbidenScope to anywhere you think is "safe" to
prevent any future script from being added. It's the only way to be sure.

[robwu, 2016-02-10 00:51:27]

I have been experimenting with microtasks, and I see more disadvantages than
advantages.

Because the scripts have to be injected before the rest of the document is
parsed, there is no change from Blink's perspective.

The only benefit is that the RenderFrame observers are not invalidated while
notifying the observers.

The downsides are:
1. If the parser is activated while the microtask queue is being drained, then
the content script is NOT "immediately" injected (because nested microtasks are
not possible).
2. Forcing a microtask checkpoint might have side effects.
3. Bookkeeping of injection state in the script injection manager is more
complex.

Do you still want to go down the route of microtasks, or is the original method
(patch set 4) okay?

[esprehn, 2016-02-10 01:08:57]

esprehn@chromium.org changed reviewers:
+ kouhei@chromium.org

[esprehn, 2016-02-10 01:08:57]

I have to think about this more, in the meantime you should get kouhei to look
at the parser changes.

[kouhei (in TOK), 2016-02-10 01:21:19]

On 2016/02/10 01:08:57, esprehn wrote:
> I have to think about this more, in the meantime you should get kouhei to look
> at the parser changes.

Would you add me to the bug? It's hard to see why the change is made.

[kouhei (in TOK), 2016-02-10 01:38:01]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org

[kouhei (in TOK), 2016-02-10 01:38:02]

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/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:2296: if
(m_parser->isStopped()) {
I don't understand the intention of the change here. Would you describe this in
detail?

[haraken, 2016-02-10 01:47:03]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2016-02-10 01:47:03]

+yhirano@ for Micotask::performCheckpoint

[yhirano, 2016-02-10 01:51:50]

On 2016/02/10 01:47:03, haraken wrote:
> +yhirano@ for Micotask::performCheckpoint

I cannot see the crbug issue: Can you cc me?

[robwu, 2016-02-10 10:19:52]

https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/1642283002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:2296: if
(m_parser->isStopped()) {
On 2016/02/10 01:38:02, kouhei wrote:
> I don't understand the intention of the change here. Would you describe this
in
> detail?

When the HTML document consists of text only, e.g. as in <iframe
srcdoc="text"></iframe>, then we fall through to to "case InHeadMode" below,
which calls m_tree.insertTextNode
(https://cs.chromium.org/HTMLConstructionSite%3A%3AinsertTextNode%20file%3Acpp),
which populates m_pendingText. The destructor of HTMLConstructionSite asserts
whether m_pendingText is empty
(https://cs.chromium.org/HTMLConstructionSite%3A%3A~HTMLConstructionSite%20fil...).
This assertion is triggered if we don't return early.

Now, before returning the buffer must also be drained, or an assertion would be
triggered in ~CharacterTokenBuffer:
https://cs.chromium.org/~CharacterTokenBuffer

I found these assertions when I ran the unit tests that I introduced with this
CL.

[yhirano, 2016-02-10 18:32:21]

Description was changed from

==========
Deal with frame removal by content scripts

BUG=582008
==========

to

==========
Deal with frame removal by content scripts

BUG=582008
==========

[yhirano, 2016-02-10 18:50:42]

I made the CL protected and removed auto-cc.

[yhirano, 2016-02-10 19:53:46]

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, adding an explicit
output task parameter so that the caller can execute that task after
RenderFrameObserver::DidCreateDocumentElement returns is more preferable than
relying on Microtask.

[robwu, 2016-02-10 20:33:08]

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 don't want to execute
> scripts in RenderFrameObserver::DidCreateDocumentElement, adding an explicit
> output task parameter so that the caller can execute that task after
> RenderFrameObserver::DidCreateDocumentElement returns is more preferable than
> relying on Microtask.

I'm in favor of not adding an extra out parameter, because not calling the
DidCreateDocumentElement / didFinishDocumentLoad notifications for frames that
are going to be deleted is not that harmful. And besides, frame deletion is
extremely rare.

Nasko, WDYT?

[nasko, 2016-02-10 22:25:20]

On 2016/02/10 20:33:08, robwu wrote:
> 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 don't want to
execute
> > scripts in RenderFrameObserver::DidCreateDocumentElement, adding an explicit
> > output task parameter so that the caller can execute that task after
> > RenderFrameObserver::DidCreateDocumentElement returns is more preferable
than
> > relying on Microtask.
> 
> I'm in favor of not adding an extra out parameter, because not calling the
> DidCreateDocumentElement / didFinishDocumentLoad notifications for frames that
> are going to be deleted is not that harmful. And besides, frame deletion is
> extremely rare.
> 
> Nasko, WDYT?

Why do we care about the frequency of the event? I don't think it matters, as if
it can happen, it means code needs to handle it.

In general, observer APIs are not meant to change state RFH lifetime. Is it
possible to plumb this through a different interface, something like
ChromeRendererClient, then verify the frame exist before continuing? 
We have been trying to remove cases of frames being deleted during observer
notifications. Explicitly doing it is a backwards step IMHO.

[robwu, 2016-02-10 22:38:58]

On 2016/02/10 22:25:20, nasko wrote:
> On 2016/02/10 20:33:08, robwu wrote:
> > 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 don't want to
> execute
> > > scripts in RenderFrameObserver::DidCreateDocumentElement, adding an
explicit
> > > output task parameter so that the caller can execute that task after
> > > RenderFrameObserver::DidCreateDocumentElement returns is more preferable
> than
> > > relying on Microtask.
> > 
> > I'm in favor of not adding an extra out parameter, because not calling the
> > DidCreateDocumentElement / didFinishDocumentLoad notifications for frames
that
> > are going to be deleted is not that harmful. And besides, frame deletion is
> > extremely rare.
> > 
> > Nasko, WDYT?
> 
> Why do we care about the frequency of the event? I don't think it matters, as
if
> it can happen, it means code needs to handle it.
> 
> In general, observer APIs are not meant to change state RFH lifetime. Is it
> possible to plumb this through a different interface, something like
> ChromeRendererClient, then verify the frame exist before continuing? 
> We have been trying to remove cases of frames being deleted during observer
> notifications. Explicitly doing it is a backwards step IMHO.

So you prefer something like this?

std::vector<base::Callback<void(RenderFrame)> > tasks;
FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
DidCreateDocumentElement(&tasks));

if (!tasks.empty()) {
  base::WeakPtr<RenderFrameImpl> self = weak_factory_.GetWeakPtr();
  for (auto& task : tasks) {
    task.Run(this);
    if (!self.get())
      return;
  }
}

[nasko, 2016-02-10 22:50:57]

On 2016/02/10 22:38:58, robwu wrote:
> On 2016/02/10 22:25:20, nasko wrote:
> > On 2016/02/10 20:33:08, robwu wrote:
> > > 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 don't want to
> > execute
> > > > scripts in RenderFrameObserver::DidCreateDocumentElement, adding an
> explicit
> > > > output task parameter so that the caller can execute that task after
> > > > RenderFrameObserver::DidCreateDocumentElement returns is more preferable
> > than
> > > > relying on Microtask.
> > > 
> > > I'm in favor of not adding an extra out parameter, because not calling the
> > > DidCreateDocumentElement / didFinishDocumentLoad notifications for frames
> that
> > > are going to be deleted is not that harmful. And besides, frame deletion
is
> > > extremely rare.
> > > 
> > > Nasko, WDYT?
> > 
> > Why do we care about the frequency of the event? I don't think it matters,
as
> if
> > it can happen, it means code needs to handle it.
> > 
> > In general, observer APIs are not meant to change state RFH lifetime. Is it
> > possible to plumb this through a different interface, something like
> > ChromeRendererClient, then verify the frame exist before continuing? 
> > We have been trying to remove cases of frames being deleted during observer
> > notifications. Explicitly doing it is a backwards step IMHO.
> 
> So you prefer something like this?
> 
> std::vector<base::Callback<void(RenderFrame)> > tasks;
> FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
> DidCreateDocumentElement(&tasks));
> 
> if (!tasks.empty()) {
>   base::WeakPtr<RenderFrameImpl> self = weak_factory_.GetWeakPtr();
>   for (auto& task : tasks) {
>     task.Run(this);
>     if (!self.get())
>       return;
>   }
> }

Hmm, no. Why do we need tasks here? What I had in mind is something like this:

... // existing observer loop can be before or after the following code ...

base::WeakPtr<RenderFrameImpl> self = weak_factory_.GetWeakPtr();
GetContentClient()->renderer()->CreatedDocument(frame_);
if (!self.get())
  return;

Then this CreateDocument API can be documented that it is allowed to delete the
RenderFrame. In the chrome/ layer, it can be directed over to the extensions
code, which will move whatever handling it has today from the observer method to
this one.

The ContentRendererClient is an interface that is used to alter behavior
already. It makes a lot more sense to go through it, even though at the end of
the day the exact same code will execute as part of the extensions/ handling of
the event.

[esprehn, 2016-02-10 23:06:11]

I'd prefer we didn't do that, I want this to be the only case we ever do run
script. I'd much prefer we do something like tasks and then add a
WebScriptForbiddenScope to the FOR_EACH_OBSERVER notification macro.

[nasko, 2016-02-10 23:25:38]

On 2016/02/10 23:06:11, esprehn wrote:
> I'd prefer we didn't do that, I want this to be the only case we ever do run
> script. I'd much prefer we do something like tasks and then add a
> WebScriptForbiddenScope to the FOR_EACH_OBSERVER notification macro.

Why aren't the two approaches compatible? We can still add
WebScriptForbiddenScope around observer notifications, I'm all for it. I'm
basically saying that we formalize the only case where we run scripts into a
separate ContentRendererClient API. What's the part about tasks that I'm missing
and cannot be done from the extensions/ code?

[esprehn, 2016-02-10 23:53:03]

On 2016/02/10 at 23:25:38, nasko wrote:
> On 2016/02/10 23:06:11, esprehn wrote:
> > I'd prefer we didn't do that, I want this to be the only case we ever do run
> > script. I'd much prefer we do something like tasks and then add a
> > WebScriptForbiddenScope to the FOR_EACH_OBSERVER notification macro.
> 
> Why aren't the two approaches compatible? We can still add
WebScriptForbiddenScope around observer notifications, I'm all for it. I'm
basically saying that we formalize the only case where we run scripts into a
separate ContentRendererClient API. What's the part about tasks that I'm missing
and cannot be done from the extensions/ code?

I'm fine with that approach too. I'm just saying I don't want anyone else inside
documentElementAvailable() to run script, this should be the only case, no new
instances should be added. :)

[nasko, 2016-02-11 00:08:28]

On 2016/02/10 23:53:03, esprehn wrote:
> On 2016/02/10 at 23:25:38, nasko wrote:
> > On 2016/02/10 23:06:11, esprehn wrote:
> > > I'd prefer we didn't do that, I want this to be the only case we ever do
run
> > > script. I'd much prefer we do something like tasks and then add a
> > > WebScriptForbiddenScope to the FOR_EACH_OBSERVER notification macro.
> > 
> > Why aren't the two approaches compatible? We can still add
> WebScriptForbiddenScope around observer notifications, I'm all for it. I'm
> basically saying that we formalize the only case where we run scripts into a
> separate ContentRendererClient API. What's the part about tasks that I'm
missing
> and cannot be done from the extensions/ code?
> 
> I'm fine with that approach too. I'm just saying I don't want anyone else
inside
> documentElementAvailable() to run script, this should be the only case, no new
> instances should be added. :)

Great! I wasn't sure what part was concerning you when you said "I'd prefer we
didn't do that". It sounds like we are in agreement that we need to restrict
script running to only this case and try to prevent the rest.

[robwu, 2016-02-11 12:40:24]

I routed the script injection of all extension code via the
ContentRendererClient (except for the RenderFrameObserver in
extensions/renderer/app_window_custom_bindings.cc because it appears to be dead
code).

And I didn't add WebForbiddenScope around the observers because it is not
required for this patch and there is a lot going on in DidFinishDocumentLoad.
E.g. MojoBindingsController::DidFinishDocumentLoad seems to run some JavaScript,
and adding a WebForbiddenScope around the observer without understanding all
leaves introduces the risk of breaking functionality (and might impede merging
this patch with M-49 / M-48).

[nasko, 2016-02-11 16:49:01]

https://codereview.chromium.org/1642283002/diff/140001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1642283002/diff/140001/content/public/rendere...
content/public/renderer/content_renderer_client.h:311: virtual void
AfterDidCreateDocumentElement(
Why the "After" prefix? It is called from DidCreateDocumentElement, right?

I also wonder if there is a way to avoid using a WeakPtr as an argument to this
method.

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3384: // Do not use |this| or |frame|!
ContentClient may might deleted them by now!
nit: "may might deleted" doesn't sound right. Maybe "ContentClient may have
deleted this object."

[robwu, 2016-02-11 17:51:09]

https://codereview.chromium.org/1642283002/diff/140001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1642283002/diff/140001/content/public/rendere...
content/public/renderer/content_renderer_client.h:311: virtual void
AfterDidCreateDocumentElement(
On 2016/02/11 16:49:01, nasko wrote:
> Why the "After" prefix? It is called from DidCreateDocumentElement, right?

It is called after the DidCreateDocumentElement observers have been notified.

> I also wonder if there is a way to avoid using a WeakPtr as an argument to
this
> method.

It is possible, by inheriting from SupportsWeakPtr<RenderFrame> instead of
making the WeakPtrFactory a member of RenderFrame. Callees can then use
render_frame->AsWeakPtr().

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3384: // Do not use |this| or |frame|!
ContentClient may might deleted them by now!
On 2016/02/11 16:49:01, nasko wrote:
> nit: "may might deleted" doesn't sound right. Maybe "ContentClient may have
> deleted this object."

Oops. I intended to write "might have". I'll fix it with the next patchset.

[ncarter (slow), 2016-02-11 22:47:31]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2016-02-11 22:47:32]

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:507:
ExtensionFrameHelper::Get(render_frame.get());
Nasko pointed me at this CL to see if there might be an alternative to minting
the WeakPtr on the content side and passing it through here. 

One idea would be to add a WeakPtrFactory<> to the ExtensionFrameHelper (whose
lifetime, I think, is bound to the renderframe). This would let the whole
WeakPtr dance be contained entirely inside the //extensions layer.

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:138: return;  // Frame invalidated
by callback.
That also means that |this| was deleted (right?).

I'm not super familiar with this code, but as an aesthetic measure, it might be
worth considering moving this loop out into calling code (i.e. expose something
like a "vector<Callback> GetAndClearDocumentElementCreatedCallbacks()").

[robwu, 2016-02-11 23:36:51]

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3384: // Do not use |this| or |frame|!
ContentClient may might deleted them by now!
On 2016/02/11 17:51:08, robwu wrote:
> On 2016/02/11 16:49:01, nasko wrote:
> > nit: "may might deleted" doesn't sound right. Maybe "ContentClient may have
> > deleted this object."
> 
> Oops. I intended to write "might have". I'll fix it with the next patchset.

Done.

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:507:
ExtensionFrameHelper::Get(render_frame.get());
On 2016/02/11 22:47:32, ncarter wrote:
> Nasko pointed me at this CL to see if there might be an alternative to minting
> the WeakPtr on the content side and passing it through here. 
> 
> One idea would be to add a WeakPtrFactory<> to the ExtensionFrameHelper (whose
> lifetime, I think, is bound to the renderframe). This would let the whole
> WeakPtr dance be contained entirely inside the //extensions layer.

Done. I don't see any issue with passing a weakptr down from content/ (except
that the creation of the weak ptr is unnecessary if extensions are disabled),
but it's your call.

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:138: return;  // Frame invalidated
by callback.
On 2016/02/11 22:47:32, ncarter wrote:
> That also means that |this| was deleted (right?).

Yes.

> I'm not super familiar with this code, but as an aesthetic measure, it might
be
> worth considering moving this loop out into calling code (i.e. expose
something
> like a "vector<Callback> GetAndClearDocumentElementCreatedCallbacks()").
> 

I prefer to keep it in this class, since then all (frame-specific!) callback
registration and running logic is within this class.

[ncarter (slow), 2016-02-12 19:32:57]

lgtm

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:507:
ExtensionFrameHelper::Get(render_frame.get());
On 2016/02/11 23:36:50, robwu wrote:
> On 2016/02/11 22:47:32, ncarter wrote:
> > Nasko pointed me at this CL to see if there might be an alternative to
minting
> > the WeakPtr on the content side and passing it through here. 
> > 
> > One idea would be to add a WeakPtrFactory<> to the ExtensionFrameHelper
(whose
> > lifetime, I think, is bound to the renderframe). This would let the whole
> > WeakPtr dance be contained entirely inside the //extensions layer.
> 
> Done. I don't see any issue with passing a weakptr down from content/ (except
> that the creation of the weak ptr is unnecessary if extensions are disabled),
> but it's your call.

I'm not opposed to it per se either, but I think it's cleaner if we handle the
deletion-detection at the same layer that the deletion-triggering happens.

My reasoning is that passing a weakptr as an in-param fairly uncommon idiom
outside of ctors and functions used as callbacks (where time might pass between
the bind and the invocation). Uncommon idioms result in code that's harder to
understand: in the case of the content client calls, when I first saw them, I
wondered "Why would content need to pass a WeakPtr here? Is it possible the
frame has already been deleted? If so why would content be reporting an event on
the deleted frame?"

If RenderFrameHost had already exposed a public GetWeakPtr<> method I would have
had no trouble with you using that. But it wasn't there already, and since there
is a way to get the behavior you need without adding a method to
RenderFrameHost, that's what I suggested.

[robwu, 2016-02-12 19:51:42]

rob@robwu.nl changed required reviewers:
+ kouhei@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org

[robwu, 2016-02-12 19:51:43]

Devlin: Could you review extensions/?
content/ has been approved, so I don't expect major changes in extensions code.

If you'd like to, you can skip reviewing the tests until the Blink review by
kouhei is done, since the tests are designed to cover all relevant code paths
through Blink (which is still under review).

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:507:
ExtensionFrameHelper::Get(render_frame.get());
On 2016/02/12 19:32:57, ncarter wrote:
> On 2016/02/11 23:36:50, robwu wrote:
> > On 2016/02/11 22:47:32, ncarter wrote:
> > > Nasko pointed me at this CL to see if there might be an alternative to
> minting
> > > the WeakPtr on the content side and passing it through here. 
> > > 
> > > One idea would be to add a WeakPtrFactory<> to the ExtensionFrameHelper
> (whose
> > > lifetime, I think, is bound to the renderframe). This would let the whole
> > > WeakPtr dance be contained entirely inside the //extensions layer.
> > 
> > Done. I don't see any issue with passing a weakptr down from content/
(except
> > that the creation of the weak ptr is unnecessary if extensions are
disabled),
> > but it's your call.
> 
> I'm not opposed to it per se either, but I think it's cleaner if we handle the
> deletion-detection at the same layer that the deletion-triggering happens.
> 
> My reasoning is that passing a weakptr as an in-param fairly uncommon idiom
> outside of ctors and functions used as callbacks (where time might pass
between
> the bind and the invocation). Uncommon idioms result in code that's harder to
> understand: in the case of the content client calls, when I first saw them, I
> wondered "Why would content need to pass a WeakPtr here? Is it possible the
> frame has already been deleted? If so why would content be reporting an event
on
> the deleted frame?"
> 
> If RenderFrameHost had already exposed a public GetWeakPtr<> method I would
have
> had no trouble with you using that. But it wasn't there already, and since
there
> is a way to get the behavior you need without adding a method to
> RenderFrameHost, that's what I suggested.

That makes sense. Thanks for explaining your line of reasoning and review!

[Devlin, 2016-02-12 22:21:56]

haven't checked the tests yet.

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1393:
->AfterDidCreateDocumentElement(render_frame);
hmm... I'd say we should probably mention that |render_frame| can be invalid
after this point (I know it's mentioned elsewhere, but it's good to have it at
the main entry points).

Unfortunately, there's also not really a good way of *checking* if the render
frame is alive at this point - I suppose if we really need to, we could cache
the routing id, and then RenderFrame::FromRoutingID() if we need to (or expose
something from the extensions layer but...yuck).  I suppose I'm fine with
crossing that bridge when we come to it, but it is kind of a shame.

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:512:
script_injection_manager_->AfterDidCreateDocumentElement(render_frame);
Would it make sense to have the script injection manager register a callback
with the frame helper?  Then, there's one canonical way to do work here (and we
don't have *quite* as many lifetime checks)?

Also, would that allow us to contain ExtensionFrameHelper's weakness entirely
within itself?  (We generally prefer to just have an internal weak ptr over
SupportsWeakPtr - there are a few threads on chromium-dev that explain it pretty
well.)

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.h:111: // This method is not allowed to run
JavaScript code in the frame.
Can we DCHECK this (in general for DidCreateDocumentElement) somewhere in the
blink layer? Are we already?

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_injection_manager.cc:254: void
ScriptInjectionManager::AfterDidCreateDocumentElement(
In general, it kind of makes me sad that we can't centralize the protection for
render frames a little bit more.  We have this class, ScriptInjection, and
ExtensionFrameHelper all kinda-sorta trying to figure it out.  Unfortunately, I
don't immediately have ideas of how to make it better, either.

[robwu, 2016-02-12 22:37:13]

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1393:
->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 22:21:56, Devlin wrote:
> hmm... I'd say we should probably mention that |render_frame| can be invalid
> after this point (I know it's mentioned elsewhere, but it's good to have it at
> the main entry points).

Will do.

> Unfortunately, there's also not really a good way of *checking* if the render
> frame is alive at this point

When it is really needed, we can pass a WeakPtr<RenderFrame> from content/. But
it's not needed at the moment.

> I suppose if we really need to, we could cache
> the routing id, and then RenderFrame::FromRoutingID() if we need to (or expose
> something from the extensions layer but...yuck).  I suppose I'm fine with
> crossing that bridge when we come to it, but it is kind of a shame.

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:512:
script_injection_manager_->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 22:21:56, Devlin wrote:
> Would it make sense to have the script injection manager register a callback
> with the frame helper?

I avoided that, because ScriptInjectionManager needs to be notified of every
frame, so I optimized for minimal overhead.
The callbacks in ExtensionFrameHelper are only used to support
chrome.app.window.create, which rarely happens.

> Then, there's one canonical way to do work here (and we
> don't have *quite* as many lifetime checks)?
> 
> Also, would that allow us to contain ExtensionFrameHelper's weakness entirely
> within itself?  (We generally prefer to just have an internal weak ptr over
> SupportsWeakPtr - there are a few threads on chromium-dev that explain it
pretty
> well.)

If we follow your suggestion, then yes. An alternative to what we currently
have,
ContentRendererClient -> Dispatcher -> ExtensionFrameHost
is
ContentRendererClient -> ExtensionFrameHost -> Dispatcher

but then the following pattern would be repeated 4x (for the two AfterDidXXX
notifications, for shell and chrome clients).

ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
if (frame_helper)
  frame_helper->AfterDidXXX(render_frame);
  // AfterDidXXX might have invalidated |render_frame| and |this|.

WDYT? Should we trade self-contained WeakPtrFactory for a bit of duplication?

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.h:111: // This method is not allowed to run
JavaScript code in the frame.
On 2016/02/12 22:21:56, Devlin wrote:
> Can we DCHECK this (in general for DidCreateDocumentElement) somewhere in the
> blink layer? Are we already?

Blink has ScriptForbiddenScope, but it's not public.
I am reluctant to add this restriction now, because of the risk of breaking
functionality in a merge. See https://codereview.chromium.org/1642283002/#msg34

[Devlin, 2016-02-12 22:51:01]

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:512:
script_injection_manager_->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 22:37:13, robwu wrote:
> On 2016/02/12 22:21:56, Devlin wrote:
> > Would it make sense to have the script injection manager register a callback
> > with the frame helper?
> 
> I avoided that, because ScriptInjectionManager needs to be notified of every
> frame, so I optimized for minimal overhead.
> The callbacks in ExtensionFrameHelper are only used to support
> chrome.app.window.create, which rarely happens.

What's the optimization there?  Just avoiding creating a callback and running
it?  If that's all, I'd say let's do it anyway - base::Callback is designed to
be fast and lightweight.

> > Then, there's one canonical way to do work here (and we
> > don't have *quite* as many lifetime checks)?
> > 
> > Also, would that allow us to contain ExtensionFrameHelper's weakness
entirely
> > within itself?  (We generally prefer to just have an internal weak ptr over
> > SupportsWeakPtr - there are a few threads on chromium-dev that explain it
> pretty
> > well.)
> 
> If we follow your suggestion, then yes. An alternative to what we currently
> have,
> ContentRendererClient -> Dispatcher -> ExtensionFrameHost
> is
> ContentRendererClient -> ExtensionFrameHost -> Dispatcher
> 
> but then the following pattern would be repeated 4x (for the two AfterDidXXX
> notifications, for shell and chrome clients).
> 
> ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
> if (frame_helper)
>   frame_helper->AfterDidXXX(render_frame);
>   // AfterDidXXX might have invalidated |render_frame| and |this|.
> 
> WDYT? Should we trade self-contained WeakPtrFactory for a bit of duplication?

Nah, going into dispatcher is good.  It's what everything else does.  I'd prefer
to not expose too much more to the content client.

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.h:111: // This method is not allowed to run
JavaScript code in the frame.
On 2016/02/12 22:37:13, robwu wrote:
> On 2016/02/12 22:21:56, Devlin wrote:
> > Can we DCHECK this (in general for DidCreateDocumentElement) somewhere in
the
> > blink layer? Are we already?
> 
> Blink has ScriptForbiddenScope, but it's not public.
> I am reluctant to add this restriction now, because of the risk of breaking
> functionality in a merge. See
https://codereview.chromium.org/1642283002/#msg34

SG.  Can we put an artfully placed TODO somewhere?

[esprehn, 2016-02-12 22:52:42]

On 2016/02/12 at 22:37:13, rob wrote:
> ...
> 
> Blink has ScriptForbiddenScope, but it's not public.
> I am reluctant to add this restriction now, because of the risk of breaking
functionality in a merge. See https://codereview.chromium.org/1642283002/#msg34

It is public, there's WebScriptForbiddenScope. Please add a TODO to every render
frame observer macro usage with a link to a bug and a comment that it needs one
of those.

[dcheng, 2016-02-12 22:58:00]

On 2016/02/12 at 22:52:42, esprehn wrote:
> On 2016/02/12 at 22:37:13, rob wrote:
> > ...
> > 
> > Blink has ScriptForbiddenScope, but it's not public.
> > I am reluctant to add this restriction now, because of the risk of breaking
functionality in a merge. See https://codereview.chromium.org/1642283002/#msg34
> 
> It is public, there's WebScriptForbiddenScope. Please add a TODO to every
render frame observer macro usage with a link to a bug and a comment that it
needs one of those.

esprehn@, I think you're thinking of WebPluginScriptForbiddenScope, which is
slightly different =/

[esprehn, 2016-02-12 23:14:13]

On 2016/02/12 at 22:58:00, dcheng wrote:
> On 2016/02/12 at 22:52:42, esprehn wrote:
> > On 2016/02/12 at 22:37:13, rob wrote:
> > > ...
> > > 
> > > Blink has ScriptForbiddenScope, but it's not public.
> > > I am reluctant to add this restriction now, because of the risk of
breaking functionality in a merge. See
https://codereview.chromium.org/1642283002/#msg34
> > 
> > It is public, there's WebScriptForbiddenScope. Please add a TODO to every
render frame observer macro usage with a link to a bug and a comment that it
needs one of those.
> 
> esprehn@, I think you're thinking of WebPluginScriptForbiddenScope, which is
slightly different =/

Woops, yeah but pretty easy to add this. :)

[robwu, 2016-02-12 23:24:49]

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:512:
script_injection_manager_->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 22:51:01, Devlin wrote:
> On 2016/02/12 22:37:13, robwu wrote:
> > On 2016/02/12 22:21:56, Devlin wrote:
> > > Would it make sense to have the script injection manager register a
callback
> > > with the frame helper?
> > 
> > I avoided that, because ScriptInjectionManager needs to be notified of every
> > frame, so I optimized for minimal overhead.
> > The callbacks in ExtensionFrameHelper are only used to support
> > chrome.app.window.create, which rarely happens.
> 
> What's the optimization there?  Just avoiding creating a callback and running
> it?  If that's all, I'd say let's do it anyway - base::Callback is designed to
> be fast and lightweight.

This happens (which seems not that expensive):
1. Map RenderFrame to ExtensionFrameHelper
2. Create callback
3. Insert the callback in a vector.
4. Later: Run callback.

But if I go down this route, then for consistency I'd also have to change
DidFinishDocumentLoad to be callback-based. Otherwise there are two things to do
comparable things in the ScriptInjectionManager.

> > > Then, there's one canonical way to do work here (and we
> > > don't have *quite* as many lifetime checks)?
> > > 
> > > Also, would that allow us to contain ExtensionFrameHelper's weakness
> entirely
> > > within itself?  (We generally prefer to just have an internal weak ptr
over
> > > SupportsWeakPtr - there are a few threads on chromium-dev that explain it
> > pretty
> > > well.)
> > 
> > If we follow your suggestion, then yes. An alternative to what we currently
> > have,
> > ContentRendererClient -> Dispatcher -> ExtensionFrameHost
> > is
> > ContentRendererClient -> ExtensionFrameHost -> Dispatcher
> > 
> > but then the following pattern would be repeated 4x (for the two AfterDidXXX
> > notifications, for shell and chrome clients).
> > 
> > ExtensionFrameHelper* frame_helper =
ExtensionFrameHelper::Get(render_frame);
> > if (frame_helper)
> >   frame_helper->AfterDidXXX(render_frame);
> >   // AfterDidXXX might have invalidated |render_frame| and |this|.
> > 
> > WDYT? Should we trade self-contained WeakPtrFactory for a bit of
duplication?
> 
> Nah, going into dispatcher is good.  It's what everything else does.  I'd
prefer
> to not expose too much more to the content client.

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.h:111: // This method is not allowed to run
JavaScript code in the frame.
On 2016/02/12 22:51:01, Devlin wrote:
> On 2016/02/12 22:37:13, robwu wrote:
> > On 2016/02/12 22:21:56, Devlin wrote:
> > > Can we DCHECK this (in general for DidCreateDocumentElement) somewhere in
> the
> > > blink layer? Are we already?
> > 
> > Blink has ScriptForbiddenScope, but it's not public.
> > I am reluctant to add this restriction now, because of the risk of breaking
> > functionality in a merge. See
> https://codereview.chromium.org/1642283002/#msg34
> 
> SG.  Can we put an artfully placed TODO somewhere?

I'll add a comment at the next update to the macros at content/.

[Devlin, 2016-02-12 23:42:24]

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:512:
script_injection_manager_->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 23:24:49, robwu wrote:
> On 2016/02/12 22:51:01, Devlin wrote:
> > On 2016/02/12 22:37:13, robwu wrote:
> > > On 2016/02/12 22:21:56, Devlin wrote:
> > > > Would it make sense to have the script injection manager register a
> callback
> > > > with the frame helper?
> > > 
> > > I avoided that, because ScriptInjectionManager needs to be notified of
every
> > > frame, so I optimized for minimal overhead.
> > > The callbacks in ExtensionFrameHelper are only used to support
> > > chrome.app.window.create, which rarely happens.
> > 
> > What's the optimization there?  Just avoiding creating a callback and
running
> > it?  If that's all, I'd say let's do it anyway - base::Callback is designed
to
> > be fast and lightweight.
> 
> This happens (which seems not that expensive):
> 1. Map RenderFrame to ExtensionFrameHelper
> 2. Create callback
> 3. Insert the callback in a vector.
> 4. Later: Run callback.
> 
> But if I go down this route, then for consistency I'd also have to change
> DidFinishDocumentLoad to be callback-based. Otherwise there are two things to
do
> comparable things in the ScriptInjectionManager.
> 
> > > > Then, there's one canonical way to do work here (and we
> > > > don't have *quite* as many lifetime checks)?
> > > > 
> > > > Also, would that allow us to contain ExtensionFrameHelper's weakness
> > entirely
> > > > within itself?  (We generally prefer to just have an internal weak ptr
> over
> > > > SupportsWeakPtr - there are a few threads on chromium-dev that explain
it
> > > pretty
> > > > well.)
> > > 
> > > If we follow your suggestion, then yes. An alternative to what we
currently
> > > have,
> > > ContentRendererClient -> Dispatcher -> ExtensionFrameHost
> > > is
> > > ContentRendererClient -> ExtensionFrameHost -> Dispatcher
> > > 
> > > but then the following pattern would be repeated 4x (for the two
AfterDidXXX
> > > notifications, for shell and chrome clients).
> > > 
> > > ExtensionFrameHelper* frame_helper =
> ExtensionFrameHelper::Get(render_frame);
> > > if (frame_helper)
> > >   frame_helper->AfterDidXXX(render_frame);
> > >   // AfterDidXXX might have invalidated |render_frame| and |this|.
> > > 
> > > WDYT? Should we trade self-contained WeakPtrFactory for a bit of
> duplication?
> > 
> > Nah, going into dispatcher is good.  It's what everything else does.  I'd
> prefer
> > to not expose too much more to the content client.
> 

That sounds reasonable to me.  If we're going to start using
ExtensionFrameHelper as an indication of frame liveness, let's commit to it! :)

(As a result, we *might* be able to clean up a few of the other places in
ScriptInjectionManager, but let's hold off on those changes.)

[robwu, 2016-02-13 00:28:25]

I've added some comments to content/ and addressed all review points in patchset
10.

[ncarter (slow), 2016-02-16 18:22:32]

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1642283002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1393:
->AfterDidCreateDocumentElement(render_frame);
On 2016/02/12 22:37:13, robwu wrote:
> On 2016/02/12 22:21:56, Devlin wrote:
> > hmm... I'd say we should probably mention that |render_frame| can be invalid
> > after this point (I know it's mentioned elsewhere, but it's good to have it
at
> > the main entry points).
> 
> Will do.
> 
> > Unfortunately, there's also not really a good way of *checking* if the
render
> > frame is alive at this point
> 
> When it is really needed, we can pass a WeakPtr<RenderFrame> from content/.
But
> it's not needed at the moment.
> 
> > I suppose if we really need to, we could cache
> > the routing id, and then RenderFrame::FromRoutingID() if we need to (or
expose
> > something from the extensions layer but...yuck).  I suppose I'm fine with
> > crossing that bridge when we come to it, but it is kind of a shame.

If it comes to that, I'd rather we just expose a GetWeakPtr on RenderFrame (as
devlin points out, it's a bit redundant to the routing pair lookup).

For now, documenting up the call stack that "// |frame| may have been deleted."
(as you've done) is probably a best practice.

[Devlin, 2016-02-16 18:37:37]

Still waiting on the blink folks' lg to look at the tests in depth, but this
generally lg2m.

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.h:113: // These methods may run (untrusted)
JavaScript code in the frame, and
nit: let's put a newline after line 112.

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:158:
document_element_created_callbacks_.push_back(callback);
Can we add a pair of bools (did finish doc load, did create doc element) or an
enum to represent the state this frame is in so that we can DCHECK here that
people aren't adding callbacks that will never be called (or will be called on a
new load)?

[esprehn, 2016-02-17 01:43:47]

You need a better change description, you need to explain what you're doing and
why.

This also doesn't match what I wanted, I want to assert that
dispatchDocumentElementAvailable never runs script, you need totally separate
plumbing for extensions which is *specific* to extensions.

You need a new embedder callback like
NotifyExtensionScriptInsertedDocumentElement() and
NotifyExtensionScriptInsertedBody() or whatever might run script. All other
callbacks will be forbidden from running script.

https://codereview.chromium.org/1642283002/diff/180001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1642283002/diff/180001/content/public/rendere...
content/public/renderer/content_renderer_client.h:309: // invalidate the frame.
This should be specific to extensions, This should be
NotifyExtensionsAfterDidCreateDocumentElement.

https://codereview.chromium.org/1642283002/diff/180001/content/public/rendere...
content/public/renderer/content_renderer_client.h:314: virtual void
AfterDidFinishDocumentLoad(RenderFrame* render_frame) {}
ditto

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/PluginDocument.cpp (right):

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:102: if (isStopped())
huh?

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:113: if (isStopped())
how does inserting the body run script internally?

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:126: if (isStopped())
huh? What happens inside focus?

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:181:
RefPtrWillBeRawPtr<WebLocalFrameImpl> protector(m_webFrame.get());
protect(

not protector

[robwu, 2016-02-17 23:49:05]

On 2016/02/17 01:43:47, esprehn wrote:
> You need a better change description, you need to explain what you're doing
and
> why.

The rationale is in the bug, I initially kept the description obscure because it
was public. Now the CL is private, I'll add some descriptive message (after
we've finially decided on what approach to take. This has changed at least twice
already...).
 
> This also doesn't match what I wanted, I want to assert that
> dispatchDocumentElementAvailable never runs script, you need totally separate
> plumbing for extensions which is *specific* to extensions.

I separated plumbing for extensions in content/. Regardless of whether the
notification is triggered from Blink or content/, Blink has to account for the
possibility that the document may be destroyed after the document element is
inserted.

> You need a new embedder callback like
> NotifyExtensionScriptInsertedDocumentElement() and
> NotifyExtensionScriptInsertedBody() or whatever might run script. All other
> callbacks will be forbidden from running script.

I tracked down all callees; besides extensions there is another part that could
synchronously run scripts: MojoBindingsController::DidFinishDocumentLoad (from a
cursory look, this seems like a safe & static script). Do you want
ScriptForbiddenScope for document element insertion only, or for both
notifications? Extension-specific callback names only make sense in the former
case.

And in blink::FrameLoaderClientImpl::dispatchDidFinishDocumentLoad ,
RenderFrameImpl is not the only client, so I cannot put ScriptForbiddenScope
here, or SharedWorkers/ServiceWorkers may not function properly. So applying
ScriptForbiddenScope to the FinishDocumentLoad notification is currently not
even practical.

Regarding your other comments, once an untrusted script has been inserted, it
can use mutation events to observe element insertions/focus changes, and remove
the frame in these events.

[esprehn, 2016-02-18 00:10:55]

On 2016/02/17 at 23:49:05, rob wrote:
> On 2016/02/17 01:43:47, esprehn wrote:
> > You need a better change description, you need to explain what you're doing
and
> > why.
> 
> The rationale is in the bug, I initially kept the description obscure because
it was public. Now the CL is private, I'll add some descriptive message (after
we've finially decided on what approach to take. This has changed at least twice
already...).

I'm not going to lgtm a patch without a good description, people forget all the
time. Please do write one now and change it as you go, your lack of description
is slowing down the codereview. I can't tell what you're trying to do.

Bugs are never substitutes for a change description. Please note that security
by obscurity in the change description is also something we don't do.

>  
> > This also doesn't match what I wanted, I want to assert that
> > dispatchDocumentElementAvailable never runs script, you need totally
separate
> > plumbing for extensions which is *specific* to extensions.
> 
> I separated plumbing for extensions in content/. Regardless of whether the
notification is triggered from Blink or content/, Blink has to account for the
possibility that the document may be destroyed after the document element is
inserted.

It should be in blink, it needs to be super clear which callbacks in blink run
scripts, all others need to be locked down.

> 
> > You need a new embedder callback like
> > NotifyExtensionScriptInsertedDocumentElement() and
> > NotifyExtensionScriptInsertedBody() or whatever might run script. All other
> > callbacks will be forbidden from running script.
> 
> I tracked down all callees; besides extensions there is another part that
could synchronously run scripts: MojoBindingsController::DidFinishDocumentLoad
(from a cursory look, this seems like a safe & static script). Do you want
ScriptForbiddenScope for document element insertion only, or for both
notifications? Extension-specific callback names only make sense in the former
case.

Is that one author code? What uses Mojo JS bindings? If it's UA code it should
be marked with AllowUserAgentScript, if it's not, we need to audit what it's
doing with mojo in the main world context... 

In general I don't know what this mojo bindings code is doing, but it doesn't
seem right.

> 
> And in blink::FrameLoaderClientImpl::dispatchDidFinishDocumentLoad ,
RenderFrameImpl is not the only client, so I cannot put ScriptForbiddenScope
here, or SharedWorkers/ServiceWorkers may not function properly. 

Huh? SharedWorkers and ServiceWorkers don't have documents, why are they running
DidFinishDocumentLoad and running scripts?

> So applying ScriptForbiddenScope to the FinishDocumentLoad notification is
currently not even practical.
> 
> Regarding your other comments, once an untrusted script has been inserted, it
can use mutation events to observe element insertions/focus changes, and remove
the frame in these events.

Huh? The parser doesn't fire mutation events, it does run script in a couple
cases though which is handled inside parserAppendChild and parserRemoveChild.
What case are you thinking of?

[esprehn, 2016-02-18 00:11:17]

sigh, not lgtm, forgot I can't say it :)

[robwu, 2016-02-18 01:05:31]

Description was changed from

==========
Deal with frame removal by content scripts

BUG=582008
==========

to

==========
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
==========

[robwu, 2016-02-18 01:20:36]

On 2016/02/18 00:10:55, esprehn wrote:
> On 2016/02/17 at 23:49:05, rob wrote:
> > On 2016/02/17 01:43:47, esprehn wrote:
> > > You need a better change description, you need to explain what you're
doing
> and
> > > why.
> > 
> > The rationale is in the bug, I initially kept the description obscure
because
> it was public. Now the CL is private, I'll add some descriptive message (after
> we've finially decided on what approach to take. This has changed at least
twice
> already...).
> 
> I'm not going to lgtm a patch without a good description, people forget all
the
> time. Please do write one now and change it as you go, your lack of
description
> is slowing down the codereview. I can't tell what you're trying to do.
> 
> Bugs are never substitutes for a change description. Please note that security
> by obscurity in the change description is also something we don't do.

Done.

> > > This also doesn't match what I wanted, I want to assert that
> > > dispatchDocumentElementAvailable never runs script, you need totally
> separate
> > > plumbing for extensions which is *specific* to extensions.
> > 
> > I separated plumbing for extensions in content/. Regardless of whether the
> notification is triggered from Blink or content/, Blink has to account for the
> possibility that the document may be destroyed after the document element is
> inserted.
> 
> It should be in blink, it needs to be super clear which callbacks in blink run
> scripts, all others need to be locked down.> > > 
> > > You need a new embedder callback like
> > > NotifyExtensionScriptInsertedDocumentElement() and
> > > NotifyExtensionScriptInsertedBody() or whatever might run script. All
other
> > > callbacks will be forbidden from running script.
> > 
> > I tracked down all callees; besides extensions there is another part that
> could synchronously run scripts: MojoBindingsController::DidFinishDocumentLoad
> (from a cursory look, this seems like a safe & static script). Do you want
> ScriptForbiddenScope for document element insertion only, or for both
> notifications? Extension-specific callback names only make sense in the former
> case.
> 
> Is that one author code? What uses Mojo JS bindings? If it's UA code it should
> be marked with AllowUserAgentScript, if it's not, we need to audit what it's
> doing with mojo in the main world context... 

Idk, I'll study the code in more detail or ask the code owner to see what's
intended.

> In general I don't know what this mojo bindings code is doing, but it doesn't
> seem right.
> 
> > 
> > And in blink::FrameLoaderClientImpl::dispatchDidFinishDocumentLoad ,
> RenderFrameImpl is not the only client, so I cannot put ScriptForbiddenScope
> here, or SharedWorkers/ServiceWorkers may not function properly. 
> 
> Huh? SharedWorkers and ServiceWorkers don't have documents, why are they
running
> DidFinishDocumentLoad and running scripts?

Web workers are implemented as an invisible WebFrame, so it does share dome
logic with frames containing documents.
> 
> > So applying ScriptForbiddenScope to the FinishDocumentLoad notification is
> currently not even practical.
> > 
> > Regarding your other comments, once an untrusted script has been inserted,
it
> can use mutation events to observe element insertions/focus changes, and
remove
> the frame in these events.
>
> Huh? The parser doesn't fire mutation events, it does run script in a couple
> cases though which is handled inside parserAppendChild and parserRemoveChild.
> What case are you thinking of?

E.g. In PluginDocument, ContainerNode::appendChild fires mutation events at the
end. I also ran unit tests without my Blink patches and observed crashes in the
tests using mutation events.

[dcheng, 2016-02-18 01:35:45]

On 2016/02/18 at 01:20:36, rob wrote:
> On 2016/02/18 00:10:55, esprehn wrote:
> > On 2016/02/17 at 23:49:05, rob wrote:
> > > On 2016/02/17 01:43:47, esprehn wrote:
> > > > You need a better change description, you need to explain what you're
doing
> > and
> > > > why.
> > > 
> > > The rationale is in the bug, I initially kept the description obscure
because
> > it was public. Now the CL is private, I'll add some descriptive message
(after
> > we've finially decided on what approach to take. This has changed at least
twice
> > already...).
> > 
> > I'm not going to lgtm a patch without a good description, people forget all
the
> > time. Please do write one now and change it as you go, your lack of
description
> > is slowing down the codereview. I can't tell what you're trying to do.
> > 
> > Bugs are never substitutes for a change description. Please note that
security
> > by obscurity in the change description is also something we don't do.
> 
> Done.
> 
> > > > This also doesn't match what I wanted, I want to assert that
> > > > dispatchDocumentElementAvailable never runs script, you need totally
> > separate
> > > > plumbing for extensions which is *specific* to extensions.
> > > 
> > > I separated plumbing for extensions in content/. Regardless of whether the
> > notification is triggered from Blink or content/, Blink has to account for
the
> > possibility that the document may be destroyed after the document element is
> > inserted.
> > 
> > It should be in blink, it needs to be super clear which callbacks in blink
run
> > scripts, all others need to be locked down.> > > 
> > > > You need a new embedder callback like
> > > > NotifyExtensionScriptInsertedDocumentElement() and
> > > > NotifyExtensionScriptInsertedBody() or whatever might run script. All
other
> > > > callbacks will be forbidden from running script.
> > > 
> > > I tracked down all callees; besides extensions there is another part that
> > could synchronously run scripts:
MojoBindingsController::DidFinishDocumentLoad
> > (from a cursory look, this seems like a safe & static script). Do you want
> > ScriptForbiddenScope for document element insertion only, or for both
> > notifications? Extension-specific callback names only make sense in the
former
> > case.
> > 
> > Is that one author code? What uses Mojo JS bindings? If it's UA code it
should
> > be marked with AllowUserAgentScript, if it's not, we need to audit what it's
> > doing with mojo in the main world context... 
> 
> Idk, I'll study the code in more detail or ask the code owner to see what's
intended.

I'll follow up on this (I'm prepping an email to send to chromium-mojo@), there
are a lot of existing warts unfortunately.

> 
> > In general I don't know what this mojo bindings code is doing, but it
doesn't
> > seem right.
> > 
> > > 
> > > And in blink::FrameLoaderClientImpl::dispatchDidFinishDocumentLoad ,
> > RenderFrameImpl is not the only client, so I cannot put ScriptForbiddenScope
> > here, or SharedWorkers/ServiceWorkers may not function properly. 
> > 
> > Huh? SharedWorkers and ServiceWorkers don't have documents, why are they
running
> > DidFinishDocumentLoad and running scripts?
> 
> Web workers are implemented as an invisible WebFrame, so it does share dome
logic with frames containing documents.

Yeah, also a known wart: https://crbug.com/538751 

> > 
> > > So applying ScriptForbiddenScope to the FinishDocumentLoad notification is
> > currently not even practical.
> > > 
> > > Regarding your other comments, once an untrusted script has been inserted,
it
> > can use mutation events to observe element insertions/focus changes, and
remove
> > the frame in these events.
> >
> > Huh? The parser doesn't fire mutation events, it does run script in a couple
> > cases though which is handled inside parserAppendChild and
parserRemoveChild.
> > What case are you thinking of?
> 
> E.g. In PluginDocument, ContainerNode::appendChild fires mutation events at
the end. I also ran unit tests without my Blink patches and observed crashes in
the tests using mutation events.

[robwu, 2016-02-19 22:55:22]

This CL is blocked on replies from esprehn and dcheng.

esprehn: In comment #55, I asked
> Do you want ScriptForbiddenScope for document element insertion only,
> or for both notifications? Extension-specific callback names only make
> sense in the former case.

I think that the answer is "No", because Web workers would otherwise break, but
please confirm.

dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously run a
callback that injects a script.
- Is it important to run the callback synchronously (i.e. is it not possible to
use a PostTask)?
- If yes, is the code safe for use with AllowUserAgentScript.
I can also move MojoBindingsController::DidFinishDocumentLoad to the new
AfterDidFinishDocumentLoad (name is subject to change) notification, which
allows scripts to run, and also takes into account that scripts may invalidate
the frame.

[esprehn, 2016-02-19 23:02:47]

I'm loooking at the code for DidFinishDocumentLoad for workers, it doesn't seem
to run any script sync, what fails here?

[dcheng, 2016-02-19 23:06:33]

On 2016/02/19 at 22:55:22, rob wrote:
> This CL is blocked on replies from esprehn and dcheng.
> 
> esprehn: In comment #55, I asked
> > Do you want ScriptForbiddenScope for document element insertion only,
> > or for both notifications? Extension-specific callback names only make
> > sense in the former case.
> 
> I think that the answer is "No", because Web workers would otherwise break,
but please confirm.
> 
> dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously run a
callback that injects a script.
> - Is it important to run the callback synchronously (i.e. is it not possible
to use a PostTask)?
> - If yes, is the code safe for use with AllowUserAgentScript.
> I can also move MojoBindingsController::DidFinishDocumentLoad to the new
AfterDidFinishDocumentLoad (name is subject to change) notification, which
allows scripts to run, and also takes into account that scripts may invalidate
the frame.

Sorry, I started up writing this mail, but just fixing it for mojo bindings
controller won't fix all the other instances where this sort of thing breaks. So
I've been looking at other violations of this sort, why they need to inject
script, and trying to write up something that covers why code does this today
(usually it's because it wants to inject script or make something available 'as
soon as possible') and try to explore better solutions for the future.

[robwu, 2016-02-19 23:32:52]

On 2016/02/19 23:02:47, esprehn wrote:
> I'm loooking at the code for DidFinishDocumentLoad for workers, it doesn't
seem
> to run any script sync, what fails here?

Thanks for the quick reply!
I took another look, and it seems that you're probably right (I looked at the
wrong overload).

I'll try to put a ScriptForbiddenScope around both notifications and cross my
fingers.
Do you insist on using an extension-specific notification name, or is a generic
AfterDidFinishDocumentLoad also fine (with potential non-extension use cases in
mind, e.g. MojoBindingsController)?

[robwu, 2016-02-19 23:41:00]

On 2016/02/19 23:06:33, dcheng wrote:
> > dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously run
a
> callback that injects a script.
> > - Is it important to run the callback synchronously (i.e. is it not possible
> to use a PostTask)?
> > - If yes, is the code safe for use with AllowUserAgentScript.
> > I can also move MojoBindingsController::DidFinishDocumentLoad to the new
> AfterDidFinishDocumentLoad (name is subject to change) notification, which
> allows scripts to run, and also takes into account that scripts may invalidate
> the frame.
> 
> Sorry, I started up writing this mail, but just fixing it for mojo bindings
> controller won't fix all the other instances where this sort of thing breaks.
So
> I've been looking at other violations of this sort, why they need to inject
> script, and trying to write up something that covers why code does this today
> (usually it's because it wants to inject script or make something available
'as
> soon as possible') and try to explore better solutions for the future.

When I looked at all derived classes of
RenderFrameObserver::DidFinishDocumentLoad, I didn't see any other non-extension
use except for MojoBindingsController. So implementing my last suggestion
doesn't seem like that difficult: I can simply do
RenderFrameObserverTracker<MojoBindingsController>::Get(frame)->TheLogicOfDidFinishDocumentLoad();
Is that OK, or am I missing something?

[dcheng, 2016-02-19 23:54:24]

dcheng@chromium.org changed reviewers:
+ rockot@chromium.org

[dcheng, 2016-02-19 23:54:25]

On 2016/02/19 at 23:41:00, rob wrote:
> On 2016/02/19 23:06:33, dcheng wrote:
> > > dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously
run a
> > callback that injects a script.
> > > - Is it important to run the callback synchronously (i.e. is it not
possible
> > to use a PostTask)?
> > > - If yes, is the code safe for use with AllowUserAgentScript.
> > > I can also move MojoBindingsController::DidFinishDocumentLoad to the new
> > AfterDidFinishDocumentLoad (name is subject to change) notification, which
> > allows scripts to run, and also takes into account that scripts may
invalidate
> > the frame.
> > 
> > Sorry, I started up writing this mail, but just fixing it for mojo bindings
> > controller won't fix all the other instances where this sort of thing
breaks. So
> > I've been looking at other violations of this sort, why they need to inject
> > script, and trying to write up something that covers why code does this
today
> > (usually it's because it wants to inject script or make something available
'as
> > soon as possible') and try to explore better solutions for the future.
> 
> When I looked at all derived classes of
RenderFrameObserver::DidFinishDocumentLoad, I didn't see any other non-extension
use except for MojoBindingsController. So implementing my last suggestion
doesn't seem like that difficult: I can simply do
RenderFrameObserverTracker<MojoBindingsController>::Get(frame)->TheLogicOfDidFinishDocumentLoad();
> Is that OK, or am I missing something?

For DidFinishDocumentLoad(), I think that might be it, but we have similar
problems in many other callbacks. I guess if you only want to address this
particular issue, I believe the answer to both of your questions should be
yes... but rockot@ should confirm.

[Ken Rockot(use gerrit already), 2016-02-21 16:03:48]

On 2016/02/19 at 23:54:25, dcheng wrote:
> On 2016/02/19 at 23:41:00, rob wrote:
> > On 2016/02/19 23:06:33, dcheng wrote:
> > > > dcheng: MojoBindingsController::DidFinishDocumentLoad may synchronously
run a
> > > callback that injects a script.
> > > > - Is it important to run the callback synchronously (i.e. is it not
possible
> > > to use a PostTask)?
> > > > - If yes, is the code safe for use with AllowUserAgentScript.
> > > > I can also move MojoBindingsController::DidFinishDocumentLoad to the new
> > > AfterDidFinishDocumentLoad (name is subject to change) notification, which
> > > allows scripts to run, and also takes into account that scripts may
invalidate
> > > the frame.
> > > 
> > > Sorry, I started up writing this mail, but just fixing it for mojo
bindings
> > > controller won't fix all the other instances where this sort of thing
breaks. So
> > > I've been looking at other violations of this sort, why they need to
inject
> > > script, and trying to write up something that covers why code does this
today
> > > (usually it's because it wants to inject script or make something
available 'as
> > > soon as possible') and try to explore better solutions for the future.
> > 
> > When I looked at all derived classes of
RenderFrameObserver::DidFinishDocumentLoad, I didn't see any other non-extension
use except for MojoBindingsController. So implementing my last suggestion
doesn't seem like that difficult: I can simply do
RenderFrameObserverTracker<MojoBindingsController>::Get(frame)->TheLogicOfDidFinishDocumentLoad();
> > Is that OK, or am I missing something?
> 
> For DidFinishDocumentLoad(), I think that might be it, but we have similar
problems in many other callbacks. I guess if you only want to address this
particular issue, I believe the answer to both of your questions should be
yes... but rockot@ should confirm.

There's nothing wrong with moving this logic around as suggested

[robwu, 2016-02-22 00:15:02]

Thanks for your confirmation rockot@.

esprehn: I've added ScriptForbiddenScope in FrameLoader (2x) and new Blink
callbacks (runScriptsAtDocumentElementAvailable / runScriptsAtDocumentReady)
that can run scripts. Could you take a look?

content/ and extensions/ were also changed, because I renamed AfterDidXX to
RunScriptsAtDocumentStart/End (because the notification uses a completely
different Blink notification, I didn't want to couple the naming with the
existing notification, and I also wanted to emphasize that the callback is
intended for logic that runs scripts).

I briefly made this bug public to run the try bots. Some bots are red, but
that's because I marked the CL private again too soon. Then I made the CL public
until the bot_update steps completed before making the bug private again. All
remaining bots became green, which means that the latest change (introducing
ScriptForbiddenScope) did not knowingly break any functionality
(win_chromium_rel_ng and linux_chromium_rel_ng both cover extension tests and
Blink, so that alone would be sufficient).

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:158:
document_element_created_callbacks_.push_back(callback);
On 2016/02/16 18:37:37, Devlin wrote:
> Can we add a pair of bools (did finish doc load, did create doc element) or an
> enum to represent the state this frame is in so that we can DCHECK here that
> people aren't adding callbacks that will never be called

No. The re-entrancy via JavaScript and Blink makes it not possible to assert
things like DCHECK(!did_create_current_document_element_). It might be possible
for the load event, but that is not guaranteed by any contract in Blink.

Because running the scripts now happens in a separate notification, we could
change the implementation to only run the script once per document, but that is
not required for fixing this bug (and a huge undertaking in itself), so I'm not
going to block this CL on that.

> (or will be called on a new load)?

Until there exist a "frame is unloaded" notification from Blink, I cannot really
determine whether this holds. ScriptInjectionManager::RFOHelper attempts to
answer this, but that is in very specific to content scripts and not suitable
for general use.

ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
notification (this is clearly documented in the header), and I think that it's
sufficient for now.

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/PluginDocument.cpp (right):

https://codereview.chromium.org/1642283002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:126: if (isStopped())
On 2016/02/17 01:43:47, esprehn wrote:
> huh? What happens inside focus?

It synchronously dispatches the focus event. A potentially untrusted focus
listener could detach the frame in the focus handler.

[dcheng, 2016-02-22 00:28:34]

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
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 2016/02/16 18:37:37, Devlin wrote:
> > Can we add a pair of bools (did finish doc load, did create doc element) or
an
> > enum to represent the state this frame is in so that we can DCHECK here that
> > people aren't adding callbacks that will never be called
> 
> No. The re-entrancy via JavaScript and Blink makes it not possible to assert
things like DCHECK(!did_create_current_document_element_). It might be possible
for the load event, but that is not guaranteed by any contract in Blink.
> 
> Because running the scripts now happens in a separate notification, we could
change the implementation to only run the script once per document, but that is
not required for fixing this bug (and a huge undertaking in itself), so I'm not
going to block this CL on that.
> 
> > (or will be called on a new load)?
> 
> Until there exist a "frame is unloaded" notification from Blink, I cannot
really determine whether this holds. ScriptInjectionManager::RFOHelper attempts
to answer this, but that is in very specific to content scripts and not suitable
for general use.

FWIW this already exists in the form of WebFrameClient::frame detached

> 
> ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
notification (this is clearly documented in the header), and I think that it's
sufficient for now.

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
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 2016/02/16 18:37:37, Devlin wrote:
> > Can we add a pair of bools (did finish doc load, did create doc element) or
an
> > enum to represent the state this frame is in so that we can DCHECK here that
> > people aren't adding callbacks that will never be called
> 
> No. The re-entrancy via JavaScript and Blink makes it not possible to assert
things like DCHECK(!did_create_current_document_element_). It might be possible
for the load event, but that is not guaranteed by any contract in Blink.
> 
> Because running the scripts now happens in a separate notification, we could
change the implementation to only run the script once per document, but that is
not required for fixing this bug (and a huge undertaking in itself), so I'm not
going to block this CL on that.
> 
> > (or will be called on a new load)?
> 
> Until there exist a "frame is unloaded" notification from Blink, I cannot
really determine whether this holds. ScriptInjectionManager::RFOHelper attempts
to answer this, but that is in very specific to content scripts and not suitable
for general use.

FWIW this already exists in the form of WebFrameClient::frameDetached

> 
> ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
notification (this is clearly documented in the header), and I think that it's
sufficient for now.

[robwu, 2016-02-22 00:52:31]

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
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 at 00:15:02, robwu wrote:
> > On 2016/02/16 18:37:37, Devlin wrote:
> > > Can we add a pair of bools (did finish doc load, did create doc element)
or
> an
> > > enum to represent the state this frame is in so that we can DCHECK here
that
> > > people aren't adding callbacks that will never be called
> > 
> > No. The re-entrancy via JavaScript and Blink makes it not possible to assert
> things like DCHECK(!did_create_current_document_element_). It might be
possible
> for the load event, but that is not guaranteed by any contract in Blink.
> > 
> > Because running the scripts now happens in a separate notification, we could
> change the implementation to only run the script once per document, but that
is
> not required for fixing this bug (and a huge undertaking in itself), so I'm
not
> going to block this CL on that.
> > 
> > > (or will be called on a new load)?
> > 
> > Until there exist a "frame is unloaded" notification from Blink, I cannot
> really determine whether this holds. ScriptInjectionManager::RFOHelper
attempts
> to answer this, but that is in very specific to content scripts and not
suitable
> for general use.
> 
> FWIW this already exists in the form of WebFrameClient::frameDetached

Is that also called upon navigation? I thought that it's only for frame removal
and process swaps. By "frame unload", I'm referring to the event where the
document and script context of a frame become invalid.

> > 
> > ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
> notification (this is clearly documented in the header), and I think that it's
> sufficient for now.

[dcheng, 2016-02-22 17:51:32]

On 2016/02/22 at 00:52:31, rob wrote:
>
https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
> File extensions/renderer/extension_frame_helper.cc (right):
> 
>
https://codereview.chromium.org/1642283002/diff/180001/extensions/renderer/ex...
> 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 at 00:15:02, robwu wrote:
> > > On 2016/02/16 18:37:37, Devlin wrote:
> > > > Can we add a pair of bools (did finish doc load, did create doc element)
or
> > an
> > > > enum to represent the state this frame is in so that we can DCHECK here
that
> > > > people aren't adding callbacks that will never be called
> > > 
> > > No. The re-entrancy via JavaScript and Blink makes it not possible to
assert
> > things like DCHECK(!did_create_current_document_element_). It might be
possible
> > for the load event, but that is not guaranteed by any contract in Blink.
> > > 
> > > Because running the scripts now happens in a separate notification, we
could
> > change the implementation to only run the script once per document, but that
is
> > not required for fixing this bug (and a huge undertaking in itself), so I'm
not
> > going to block this CL on that.
> > > 
> > > > (or will be called on a new load)?
> > > 
> > > Until there exist a "frame is unloaded" notification from Blink, I cannot
> > really determine whether this holds. ScriptInjectionManager::RFOHelper
attempts
> > to answer this, but that is in very specific to content scripts and not
suitable
> > for general use.
> > 
> > FWIW this already exists in the form of WebFrameClient::frameDetached
> 
> Is that also called upon navigation? I thought that it's only for frame
removal and process swaps. By "frame unload", I'm referring to the event where
the document and script context of a frame become invalid.
> 

It's not, but navigation is less problematic in many ways: for one, it generally
doesn't cause synchronous destruction of objects. I suppose you could listen to
willReleaseScriptContext() for the latter though.

> > > 
> > > ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
> > notification (this is clearly documented in the header), and I think that
it's
> > sufficient for now.

[robwu, 2016-02-23 16:17:58]

Ping esprehn.

I was hoping to get this in the first release of M-49, but that starts to look a
bit optimistic.

I'm assuming that you'll be doing the full review (including the parser
changes). If not, please make clear who is supposed to be reviewing the parser
changes, because these parser changes are independent from the exact way of
calling the callbacks (any method requires the callback to be run before the
parser continues).

[robwu, 2016-02-26 20:49:29]

Ping esprehn.

[kouhei (in TOK), 2016-02-26 21:48:37]

re: parser changes, have we agreed that we keep the design that
dispatchDocumentElementAvailable() may invalidate the document?

[robwu, 2016-02-26 21:59:24]

On 2016/02/26 21:48:37, kouhei wrote:
> re: parser changes, have we agreed that we keep the design that
> dispatchDocumentElementAvailable() may invalidate the document?

I haven't seen any objections to that. The only issue was the ability to run
scripts in the callback, but I separated this in a notification in a
ScriptForbiddenScope, and one without.

I also tried hard to look for alternatives that did not require
dispatchDocumentElementAvailable to invalidate the document, but that is simply
not possible because extensions must run scripts right after the document
element is inserted, before the rest of the document.

[kouhei (in TOK), 2016-02-26 23:12:43]

On 2016/02/26 21:59:24, robwu wrote:
> On 2016/02/26 21:48:37, kouhei wrote:
> > re: parser changes, have we agreed that we keep the design that
> > dispatchDocumentElementAvailable() may invalidate the document?
> 
> I haven't seen any objections to that. The only issue was the ability to run
> scripts in the callback, but I separated this in a notification in a
> ScriptForbiddenScope, and one without.
> 
> I also tried hard to look for alternatives that did not require
> dispatchDocumentElementAvailable to invalidate the document, but that is
simply
> not possible because extensions must run scripts right after the document
> element is inserted, before the rest of the document.

OK. Please add comments on the parser early returns why its needed.

[esprehn, 2016-02-26 23:18:24]

On 2016/02/26 at 21:59:24, rob wrote:
> On 2016/02/26 21:48:37, kouhei wrote:
> > re: parser changes, have we agreed that we keep the design that
> > dispatchDocumentElementAvailable() may invalidate the document?
> 
> I haven't seen any objections to that. The only issue was the ability to run
scripts in the callback, but I separated this in a notification in a
ScriptForbiddenScope, and one without.
> 
> I also tried hard to look for alternatives that did not require
dispatchDocumentElementAvailable to invalidate the document, but that is simply
not possible because extensions must run scripts right after the document
element is inserted, before the rest of the document.

I objected, sorry I wasn't clear, I want the path that runs script and the path
that doesn't to be totally separate.

[robwu, 2016-02-26 23:34:56]

On 2016/02/26 23:18:24, esprehn wrote:
> On 2016/02/26 at 21:59:24, rob wrote:
> > On 2016/02/26 21:48:37, kouhei wrote:
> > > re: parser changes, have we agreed that we keep the design that
> > > dispatchDocumentElementAvailable() may invalidate the document?
> > 
> > I haven't seen any objections to that. The only issue was the ability to run
> scripts in the callback, but I separated this in a notification in a
> ScriptForbiddenScope, and one without.
> > 
> > I also tried hard to look for alternatives that did not require
> dispatchDocumentElementAvailable to invalidate the document, but that is
simply
> not possible because extensions must run scripts right after the document
> element is inserted, before the rest of the document.
> 
> I objected, sorry I wasn't clear, I want the path that runs script and the
path
> that doesn't to be totally separate.

This is only for the document element insertion
(FrameLoader::dispatchDocumentElementAvailable), right?
The one for dispatchDidFinishDocumentLoad can't be moved further without
splitting the FrameLoader::finishedParsing method.

And I hope that you're already happy with the location of ScriptForbiddenScope?

[robwu, 2016-03-05 01:50:58]

Esprehn: Please take a look. Now the dispatchDocumentElementAvailable
notification does not run any scripts at all.

kouhei: I added the comments to the early returns.

[robwu, 2016-03-09 16:54:51]

Ping for review.
@esprehn
@kouhei

[robwu, 2016-03-14 23:07:49]

Ping again... Come on, this is a security bug! Why is there no response for two
weeks?

[esprehn, 2016-03-15 00:06:52]

lgtm, looks legit.

[kouhei (in TOK), 2016-03-15 00:19:58]

lgtm for html/parser

[Devlin, 2016-03-15 00:53:32]

I took another quick look, and will look more in-depth tomorrow.

https://codereview.chromium.org/1642283002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/execute_script_apitest.cc (right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/execute_script_apitest.cc:213: ::testing::Range(0,
kNumberOfDestructiveScriptTests));
This effectively adds 108 more browser tests.  That's, frankly, too many.  I'll
look at this more closely tomorrow, but we'll have to find a way to do this
testing more efficiently.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:38:
'before the main frame\'s content script!';
nit: indentation

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:9:
chrome.test.assertTrue(TEST_HOST !== '',
nit: put TEST_HOST !== '' on the next line to match indentation with the message

[Devlin, 2016-03-15 16:34:35]

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:22:
if (event.data === 'toBeRemoved')
Do we expect any other messages?  Can this be an assert?

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:26:
// Fall back, in case the content script never ran.
If the content script never runs, shouldn't this be a failure?  Or at least an
asynchronous post to reportFrames?  I'd like to avoid a magic 3 second timeout.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:82:
doRemove();
I'm not quite sure I understand the reasoning here.  We wait for n event
firings, but if we don't see n events, we just go ahead anyway?  I'd prefer a
more deterministic test where we can wait for one specific thing, rather than "n
or some time, whichever happens first".  If we don't expect n events to occur,
we don't need that test, right?  (e.g., if we have a test for 2 events and 3
events, but we only ever get 2 events, the 3 event test is just a dup of the 2
event test.)

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:158: var
kTestsPerInstance = 2;
How slow is this if we run all 24 tests in one?

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:163: var
actualTestCount = parseInt(/count=(\d+)/.exec(location.hash)[1]);
Could we use the URL api (specifically searchParams) here instead of using a
regex?

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:182: var
url = getTestUrl(page);
Just inline this on line 212

[robwu, 2016-03-16 22:03:29]

rob@robwu.nl changed reviewers:
- haraken@chromium.org, yhirano@chromium.org

[robwu, 2016-03-16 22:03:32]

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
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:
> Do we expect any other messages?  Can this be an assert?

Done.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:26:
// Fall back, in case the content script never ran.
On 2016/03/15 16:34:35, Devlin wrote:
> If the content script never runs, shouldn't this be a failure?  Or at least an
> asynchronous post to reportFrames?  I'd like to avoid a magic 3 second
timeout.

This event was too defensive and is not necessary.
If the message never arrives due to some bug, then the test will time out and
someone will spawn a bug report.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:38:
'before the main frame\'s content script!';
On 2016/03/15 00:53:32, Devlin wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:82:
doRemove();
On 2016/03/15 16:34:34, Devlin wrote:
> I'm not quite sure I understand the reasoning here.  We wait for n event
> firings, but if we don't see n events, we just go ahead anyway?  I'd prefer a
> more deterministic test where we can wait for one specific thing, rather than
"n
> or some time, whichever happens first".  If we don't expect n events to occur,
> we don't need that test, right?  (e.g., if we have a test for 2 events and 3
> events, but we only ever get 2 events, the 3 event test is just a dup of the 2
> event test.)

I replaced the timer with a window.onload notification.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:9:
chrome.test.assertTrue(TEST_HOST !== '',
On 2016/03/15 00:53:32, Devlin wrote:
> nit: put TEST_HOST !== '' on the next line to match indentation with the
message

Done.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:158: var
kTestsPerInstance = 2;
On 2016/03/15 16:34:35, Devlin wrote:
> How slow is this if we run all 24 tests in one?

About 25 seconds. I've now disabled 6x12 DOM mutation tests at document_end
(saving about 70 seconds) and used 3 buckets instead of 12 (which reduces the
overhead of starting tests).

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:163: var
actualTestCount = parseInt(/count=(\d+)/.exec(location.hash)[1]);
On 2016/03/15 16:34:35, Devlin wrote:
> Could we use the URL api (specifically searchParams) here instead of using a
> regex?

Done.

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:182: var
url = getTestUrl(page);
On 2016/03/15 16:34:35, Devlin wrote:
> Just inline this on line 212

Done.

[Devlin, 2016-03-17 16:59:02]

Looks like this breaks webkit tests. :/

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:158: var
kTestsPerInstance = 2;
On 2016/03/16 22:03:32, robwu wrote:
> On 2016/03/15 16:34:35, Devlin wrote:
> > How slow is this if we run all 24 tests in one?
> 
> About 25 seconds. I've now disabled 6x12 DOM mutation tests at document_end
> (saving about 70 seconds) and used 3 buckets instead of 12 (which reduces the
> overhead of starting tests).

Hmm... if it's only 25 seconds, I wonder if we can just run them all in one
bucket (especially with removing the dom mutation doc end ones).  Even in the
latest patch set, it's still a bunch of added browser tests, and I'd love to get
it down to <10 added in this patch.

[robwu, 2016-03-17 22:20:40]

I've replaced DidCreateDocumentElement with RunScriptsAtDocumentStart in
MojoBindingsController to fix the crash in the WebKit tests - now all tests are
green.

For future reference, this was the stack trace leading to the crash in
V8PerIsolateData.cpp:47

#0 0x0000007d4c0e base::debug::StackTrace::StackTrace()
#1 0x000003b03a6d blink::beforeCallEnteredCallback()
#2 0x000001ce5f0d v8::internal::Isolate::FireBeforeCallEnteredCallback()
#3 0x000001a3a97a v8::Function::NewInstance()
#4 0x000003b0c40e blink::V8ScriptRunner::instantiateObject()
#5 0x000003afc9eb blink::V8ObjectConstructor::newInstance()
#6 0x000003afd60e blink::V8PerContextData::createWrapperFromCacheSlowCase()
#7 0x000003ae4cff blink::V8PerContextData::createWrapperFromCache()
#8 0x000003ae4296 blink::V8DOMWrapper::createWrapper()
#9 0x000003ac5f35 blink::ScriptWrappable::wrap()
#10 0x0000021ec7d2 blink::toV8()
#11 0x00000565bfdf WebCoreTestSupport::createInternalsObject()
#12 0x00000565c07d WebCoreTestSupport::injectInternalsObject()
#13 0x00000565beb2 blink::WebTestingSupport::injectInternalsObject()
#14 0x0000005e06b9 content::BlinkTestRunner::DidClearWindowObject()
#15 0x000004ec6bef content::RenderFrameImpl::didClearWindowObject()
#16 0x0000021e0185
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
#17 0x0000034b1b01 blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
#18 0x000003a9e9e6 blink::ScriptController::windowProxy()
#19 0x00000336662f blink::LocalFrame::windowProxy()
#20 0x000003ad8591 blink::toV8ContextEvenIfDetached()
#21 0x000003ad8430 blink::toV8Context()
#22 0x000003aacc02 blink::ScriptState::forWorld()
#23 0x000003aacb75 blink::ScriptState::forMainWorld()
#24 0x00000216c09d blink::WebLocalFrameImpl::mainWorldScriptContext()
#25 0x00000216c129 blink::WebLocalFrameImpl::mainWorldScriptContext()
#26 0x0000051a5d34 content::MojoBindingsController::CreateContextState()
#27 0x0000051a6015 content::MojoBindingsController::DidCreateDocumentElement()
#28 0x000004ec701a content::RenderFrameImpl::didCreateDocumentElement()
#29 0x0000021e03d2 blink::FrameLoaderClientImpl::documentElementAvailable()

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:158: var
kTestsPerInstance = 2;
On 2016/03/17 16:59:02, Devlin wrote:
> On 2016/03/16 22:03:32, robwu wrote:
> > On 2016/03/15 16:34:35, Devlin wrote:
> > > How slow is this if we run all 24 tests in one?
> > 
> > About 25 seconds. I've now disabled 6x12 DOM mutation tests at document_end
> > (saving about 70 seconds) and used 3 buckets instead of 12 (which reduces
the
> > overhead of starting tests).
> 
> Hmm... if it's only 25 seconds, I wonder if we can just run them all in one
> bucket (especially with removing the dom mutation doc end ones).  Even in the
> latest patch set, it's still a bunch of added browser tests, and I'd love to
get
> it down to <10 added in this patch.

I've decreased it to 1 bucket, but kept the test splitting logic in case the
bucket ever becomes too slow for a bot (and e.g. cause flaky timeouts).

[Devlin, 2016-03-18 16:46:10]

awesome, extensions lgtm.

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js
(right):

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:78:
if (performance.timing.loadEventEnd > 0)
nit: while this works, I think it'd be clearer to use document.readyState ===
'complete'.

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:164: var
filteredTests =
nit: I'm fine with leaving in most of the structure for breaking these tests up,
but can we add a quick assert here that this will run all the tests?  e.g.
assertEq(kTestsPerBucket, allTests.length).  We can easily take it out if we
adjust to have more bucketed tests.

At the same time, please add a note explaining why this is all there (in case we
need to split the tests for timing reasons).

[robwu, 2016-03-18 19:25:36]

rob@robwu.nl changed reviewers:
+ sky@chromium.org

[robwu, 2016-03-18 19:25:38]

sky: Please rs lg chrome/renderer/chrome_content_renderer_client.{h,cc}

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js
(right):

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/remove_self.js:78:
if (performance.timing.loadEventEnd > 0)
On 2016/03/18 16:46:10, Devlin wrote:
> nit: while this works, I think it'd be clearer to use document.readyState ===
> 'complete'.

Done.

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/destructive/test.js
(right):

https://codereview.chromium.org/1642283002/diff/300001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/destructive/test.js:164: var
filteredTests =
On 2016/03/18 16:46:10, Devlin wrote:
> nit: I'm fine with leaving in most of the structure for breaking these tests
up,
> but can we add a quick assert here that this will run all the tests?  e.g.
> assertEq(kTestsPerBucket, allTests.length).  We can easily take it out if we
> adjust to have more bucketed tests.

I've added an assertion that still holds even if the number of buckets changes.

> At the same time, please add a note explaining why this is all there (in case
we
> need to split the tests for timing reasons).

Done.

[sky, 2016-03-18 20:11:40]

LGTM

[robwu, 2016-03-18 21:45:39]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-03-18 21:45:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
esprehn@chromium.org, kouhei@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1642283002/#ps320001
(title: "Last nits")

[commit-bot: I haz the power, 2016-03-18 21:46:03]

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

[commit-bot: I haz the power, 2016-03-19 01:05:09]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-03-19 01:05:12]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-03-19 01:06:24]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-03-19 01:06:26]

Patchset 17 (id:??) landed as
https://crrev.com/43ea0649d4b70fdcf3e9fa5c03aee1bbba0b04bb
Cr-Commit-Position: refs/heads/master@{#382162}