Fix renderer crashes when frame gets detached while injectng user scripts.
If extension injected user script contains synchronous XHR it will cause handling of onload event for given frame. Handling this event may detach frame. Added checks if render/web view still exists for that frame after injecting extension scripts to avoid crashes.
BUG=328342
On 2013/12/13 17:55:38, jamesr wrote:
> Test?
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
I am not completely sure how to convert test case in bug to make such test.
Should it be:
1. Browsertest of RenderViewImpl that injects some js code into main frame that
creates iframe and adds helper observer class that will remove the iframe by
executing another script in DidFinishDocumentLoad?
2. Something like extension functional browser test TestAdblockExtensionCrash
that just loads prepared extension and page (although I am not sure of pass
condition here)?
3. Something else?
jamesr
I don't know what the best way to test this is, but without a regression ...
I don't know what the best way to test this is, but without a regression test
this is likely to just regress again.
mharanczyk
Added render view browsertests.
6 years, 11 months ago
(2014-01-08 08:10:29 UTC)
#6
Added render view browsertests.
mharanczyk
James, can you please give me your opinion on the tests? I prefer to make ...
6 years, 11 months ago
(2014-01-15 08:17:56 UTC)
#7
James, can you please give me your opinion on the tests?
I prefer to make it simple and to the point, without a need to use sync XHR and
extension. Security constrains prohibited me from removing frame from it parent
from frame context, that is why I used top frame.
jamesr
I don't understand your comment about sync XHR. Does this test fail without your patch? ...
6 years, 11 months ago
(2014-01-15 18:56:14 UTC)
#8
Yes, both tests cause crash and fail without patch. Regarding XHR comment: test do same ...
6 years, 11 months ago
(2014-01-16 08:57:35 UTC)
#9
Yes, both tests cause crash and fail without patch.
Regarding XHR comment: test do same thing (synchronous removal of frame on load
which caused problems) but in much simpler way than it was done in bugs TC,
which involved specific page and extension setup including doing sync XHR in
injected script to trigger fram onload handling in top page which in turn
removed frame.
mharanczyk
James, is my explanation sufficient?
6 years, 11 months ago
(2014-01-23 10:15:13 UTC)
#10
James, is my explanation sufficient?
jamesr
sorry for the delay. lgtm, but add <!DOCTYPE html> to the start of the test ...
6 years, 11 months ago
(2014-01-25 01:35:49 UTC)
#11
sorry for the delay. lgtm, but add <!DOCTYPE html> to the start of the test html
content since this isn't a quirks mode test
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mharanczyk@opera.com/112203003/190001
6 years, 11 months ago
(2014-01-27 09:47:03 UTC)
#12
Retried try job too often on mac_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=216568
6 years, 11 months ago
(2014-01-27 10:14:31 UTC)
#13
Hi! It looks like behavior changed a bit while this patch was in review, unfortunately ...
6 years, 10 months ago
(2014-01-30 11:20:36 UTC)
#14
Hi!
It looks like behavior changed a bit while this patch was in review,
unfortunately I cannot think of good way of fixing this bug so far. Problem is
that now WebFrame itself gets deleted, not only its view removed, so we are
deleting object we are notifying about in middle of notification.
Crash is reproducible with both browser tests in this patch and TC in associated
bug.
James, maybe you could point me to some reasonable solution? Is there a way to
prevent frame from being deleted to early?
jamesr
+nasko who's been following the frame lifetime changes lately. Do you have anything to suggest ...
6 years, 10 months ago
(2014-02-03 22:24:03 UTC)
#15
+nasko who's been following the frame lifetime changes lately. Do you have
anything to suggest for this problem, or know who might?
nasko
Adding dcheng@, who has been changing lifetime of Frame/WebFrame.
6 years, 10 months ago
(2014-02-03 22:49:57 UTC)
#16
Adding dcheng@, who has been changing lifetime of Frame/WebFrame.
dcheng
Nasko points out that we're calling WebFrame::executeScriptInIsolatedWorld() in a loop. Any one of these script ...
6 years, 10 months ago
(2014-02-03 23:05:48 UTC)
#17
Nasko points out that we're calling WebFrame::executeScriptInIsolatedWorld() in
a loop. Any one of these script executions could blow away frame, resulting in
deletion of WebFrame when we call WebFrame::close() in
RenderFrameImpl::frameDetached().
In the past, this wasn't a problem because WebCore::Frame actually had an owning
reference to WebFrame. In the paths that this scenario can occur, blink is aware
of this and retains a ref to Frame to prevent exactly this scenario from
happening.
Now that's no longer the case and we have potential UaF accesses of WebFrame. To
fix this, I think we actually need a blink side patch. FrameLoaderClientImpl
will need to retain a ref to WebFrame when dispatching WebFrameClient callbacks
that could blow away the WebFrame (for example, in
FrameLoaderClientImpl::dispatchDidFinishDocumentLoad).
This approach is a bit messy, and of course, this means that subsequent calls to
executeScriptInIsolatedWorld() will be no-ops (since WebFrame's reference to
WebCore::Frame is gone). This means there will be differences in behavior
depending on which order user scripts run... I don't really see a better
solution though.
mharanczyk
I already have minimal changes that fix the crash, but here are my thoughts: 1. ...
6 years, 10 months ago
(2014-02-04 10:04:13 UTC)
#18
I already have minimal changes that fix the crash, but here are my thoughts:
1. I only added protector to
FrameLoaderClientImpl::dispatchDidFinishDocumentLoad, but probably should add it
to all functions that do callbacks on client in this class, since we might not
be so sure what client will do there (now or in the future). Do you agree with
my approach on this one? Protecting frame from deletion by refing it doesn't
cost much IMHO, but it seems you wanted only specific set of functions to be
protected, the ones which are suspected to cause frame deletion.
2. Minor tweaking to RenderFrameImpl to account that it might have "null" frame
after calling observers. Here I only fixed this specific function. I haven't
found anything other misbehaving yet but I am not entirely sure if any other
part of this class should be adjusted to take frame deletion into account. Maybe
someone familiar with this code could look at it and point me to other place
where something might go wrong?
3. I fixed content::RenderViewObserver::DidFinishDocumentLoad(...) observers
code that caused crashes and assertions in browser tests and TC (autofill and
extensions user script). This part is what might cause biggest mess and
problems. RenderViewObserver implementation generally does not take into account
that it might be called on "null" frame (previous observer might have caused its
"deletion"). I fixed it for this specific case (DidFinishDocumentLoad function),
but it is all very shaky, it depends on observers registration order and what
exactly each one does(might do in future) in each of its overridden functions.
If anything changes and something bad starts happening again then in best case
we will get crashes/assertions again, in worst case unexpected behavior and wtf
bugs. I don't feel competent enough to do review of each observer and make it
deletion-proof.
To sum up: I have made fix for specific case I have found, but general problem
is out of scope of such fix, it probably quite a large task (avalanche effect).
Is it ok if I provide this simple fix and count on you to follow up if you deem
general problem worth solving?
mharanczyk
Since there was no response I've updated patch with minimal changes required to fix the ...
6 years, 10 months ago
(2014-02-07 13:28:27 UTC)
#19
Since there was no response I've updated patch with minimal changes required to
fix the crash. Also reported https://codereview.chromium.org/134643030/ for
blink changes. Please yell if you think this should be done differently.
I think I messed up rebase with changes to fix the crash in
chrome/renderer/extensions/user_script_slave.cc, sorry about that. Those are not
big though, so I hope this won't be a problem.
Jói
//components LGTM
6 years, 10 months ago
(2014-02-07 13:41:05 UTC)
#20
//components LGTM
dcheng
Wouldn't we need a similar guard for "document_idle" or "document_end" as well? (I'm not sure ...
6 years, 10 months ago
(2014-02-07 16:53:37 UTC)
#21
> Wouldn't we need a similar guard for "document_idle" or "document_end" as well? > (I'm ...
6 years, 10 months ago
(2014-02-21 08:29:32 UTC)
#22
> Wouldn't we need a similar guard for "document_idle" or "document_end" as
well?
> (I'm not sure which one this corresponds to).
Actually this patch covers "document_end" which corresponds to
didFinishDocumentLoad(...).
I failed to create a TC that would cause a crash when using "document_start",
which corresponds to DidCreateDocumentElement(...) callback. Of course that
doesn't mean there does not exist some fancy code that would synchronously
remove frame.
@dcheng Is it ok if I only create browsertest similar to the
DidFinishDocumentLoad one and assume if it passes then everything is ok in
content's RenderViewObservers as well?
"document_idle" is even more problematic because it is executed in async task
posted in UserScriptScheduler::DidFinishDocumentLoad (delayed task) and
UserScriptScheduler::DidFinishLoad() (not delayed task) and I don't have an idea
how to even write browsertest for it, not to mention actually TC that would
cause crash. But I can try to change the UserScriptScheduler::MaybeRun() code by
assuming that frame_ will be removed after call to
dispatcher_->user_script_slave()->InjectScripts(frame_,
UserScript::DOCUMENT_IDLE) (frame_ is raw pointer so we will need to ref it
before the call) and add some null checks to
UserScriptScheduler::ExecuteCodeImpl.
@dcheng, @asargent Is it ok if I go this way or you have a better idea?
>
>
https://codereview.chromium.org/112203003/diff/430001/chrome/renderer/extensi...
> File chrome/renderer/extensions/user_script_slave.cc (right):
>
>
https://codereview.chromium.org/112203003/diff/430001/chrome/renderer/extensi...
> chrome/renderer/extensions/user_script_slave.cc:353: if (top_frame) {
> top() should really never return 0... if you can wait until
> https://codereview.chromium.org/156123004/ lands, that will be the case once
> again.
>
Will do.
>
https://codereview.chromium.org/112203003/diff/430001/content/renderer/render...
> File content/renderer/render_view_impl.cc (right):
>
>
https://codereview.chromium.org/112203003/diff/430001/content/renderer/render...
> content/renderer/render_view_impl.cc:3701: if (frame->view())
> Might be good to add a comment on why you need to check this, e.g.
> // RenderViewObservers can run Javascript that removes |frame| so it may no
> longer have a view.
I have rebased a patch and this code was removed from
didFinishDocumentLoad(...), which basically now just calls observers. Do you
still want me to leave this as a warning there nevertheless to watch out for
frame usage after observers had run?
mharanczyk
I created TCs for all three injection cases and uploaded fixes for document_start and document_end. ...
6 years, 10 months ago
(2014-02-25 10:27:24 UTC)
#23
I created TCs for all three injection cases and uploaded fixes for
document_start and document_end.
Some of the code was moved to RenderFrameImpl so I have moved null checks there
and added comments.
As I suspected earlier I have a problem with document_idle. Injection happens in
a task posted by UserScriptScheduler and it uses its stored blink::WebFrame raw
pointer (frame_ field) which is hard to auto protect from deletion at that point
because it is pointer to interface. So far I have come up with two solutions,
both of which I don't really like:
1. Add ref counting to interface instead of implementation and use refptrs on
interface as protectors in UserScriptScheduler::MaybeRun().
2. Break encapsulation and static_cast to blink::WebFrameImpl and use refptrs as
protectors in UserScriptScheduler::MaybeRun().
Could you please give me your opinion on this problem?
dcheng
On 2014/02/25 10:27:24, mharanczyk wrote: > I created TCs for all three injection cases and ...
6 years, 10 months ago
(2014-02-26 06:31:41 UTC)
#24
On 2014/02/25 10:27:24, mharanczyk wrote:
> I created TCs for all three injection cases and uploaded fixes for
> document_start and document_end.
> Some of the code was moved to RenderFrameImpl so I have moved null checks
there
> and added comments.
>
> As I suspected earlier I have a problem with document_idle. Injection happens
in
> a task posted by UserScriptScheduler and it uses its stored blink::WebFrame
raw
> pointer (frame_ field) which is hard to auto protect from deletion at that
point
> because it is pointer to interface. So far I have come up with two solutions,
> both of which I don't really like:
> 1. Add ref counting to interface instead of implementation and use refptrs on
> interface as protectors in UserScriptScheduler::MaybeRun().
> 2. Break encapsulation and static_cast to blink::WebFrameImpl and use refptrs
as
> protectors in UserScriptScheduler::MaybeRun().
>
> Could you please give me your opinion on this problem?
I don't entirely understand the problem. But to reply to your immediate
question:
1) We don't want to expose refcounting in the embedding API.
2) Code outside Source/web can't reach into Source/web. It's a layering
violation and should be enforced by DEPS.
Since you've been looking at this quite a bit, would you be able to
chat/VC/whatever about this so I can understand the issues in Chrome better?
Then we can try to work towards a solution.
dcheng
https://codereview.chromium.org/112203003/diff/630001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/112203003/diff/630001/content/renderer/render_frame_impl.cc#newcode1052 content/renderer/render_frame_impl.cc:1052: base::Bind(&RenderFrameImpl::closeFrame, frame)); We can simplify this slightly: base::Bind(&WebFrame::close, base::Unretained(frame)) ...
6 years, 10 months ago
(2014-02-26 19:37:22 UTC)
#25
Issue 112203003: Fix renderer crashes when frame gets detached while injectng user scripts.
(Closed)
Created 7 years ago by mharanczyk
Modified 6 years, 1 month ago
Reviewers: Aaron Boodman, asargent_no_longer_on_chrome, jamesr, nasko, dcheng, darin (slow to review), Jói
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 6
Chromium Code Reviews
Issue
112203003:
Fix renderer crashes when frame gets detached while injectng user scripts. (Closed)
