Enable PDF accessibility when RenderFrame's accessibility mode changes.

Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
Committed: https://crrev.com/5a0018c9adcdc04c636d382f06e34bd6569ac791
Cr-Commit-Position: refs/heads/master@{#406108}

[dmazzoni, 2016-06-27 17:04:41]

Description was changed from

==========
Enable PDF accessibility when RenderFrame's accessibility mode.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
==========

to

==========
Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
==========

[dmazzoni, 2016-06-27 17:04:51]

dmazzoni@chromium.org changed reviewers:
+ nasko@chromium.org, raymes@chromium.org

[raymes, 2016-06-28 00:02:13]

Is it possible to add a test that exercises either of those cases?

https://codereview.chromium.org/2100753003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2100753003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2136: void
PepperPluginInstanceImpl::AccessibilityModeChanged() {
nit: this should go before OnDestruct

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2119: AccessibilityModeChanged());
Should we pass a bool indicating whether it is enabled/disabled so that we can
only enable it in pepper when it's enabled?

[dmazzoni, 2016-06-28 17:14:06]

> Is it possible to add a test that exercises either of those cases?

Yes, as soon as http://crrev.com/2057783002 lands
I'll add a similar test that loads the PDF first,
then enables accessibility second.

https://codereview.chromium.org/2100753003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2100753003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2136: void
PepperPluginInstanceImpl::AccessibilityModeChanged() {
On 2016/06/28 00:02:12, raymes wrote:
> nit: this should go before OnDestruct

Done.

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2119: AccessibilityModeChanged());
On 2016/06/28 00:02:12, raymes wrote:
> Should we pass a bool indicating whether it is enabled/disabled so that we can
> only enable it in pepper when it's enabled?

Doesn't seem to be needed because it can just access
render_accessibility() on the frame to check.

[nasko, 2016-06-28 21:59:51]

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:73: content::RenderFrame*
render_frame = render_view_->GetMainRenderFrame();
Are we guaranteed to have a main frame? We avoid making this assumption in the
face of out-of-process iframes, in which case the return value will be nullptr.
If it is guaranteed to exist, it might be useful to put a comment explaining
why.

[raymes, 2016-06-29 00:31:36]

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2100753003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2119: AccessibilityModeChanged());
Ahh ok - so that will only be a valid pointer if accessibility is enabled?

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:73: content::RenderFrame*
render_frame = render_view_->GetMainRenderFrame();
On 2016/06/28 21:59:51, nasko wrote:
> Are we guaranteed to have a main frame? We avoid making this assumption in the
> face of out-of-process iframes, in which case the return value will be
nullptr.
> If it is guaranteed to exist, it might be useful to put a comment explaining
> why.

Good question and I don't know the answer for sure. There are 2 cases where we
load the PDF plugin: 
1) If a PDF is loaded on a web page, we will load a browser plugin. This could
be at the top level (most commonly) or in an iframe. I'm not sure how browser
plugin works with OOPIF and I'm not sure how accessibility information gets
propagated for iframes with OOPIF so it would depend on those 2 things.

2) In print preview. In this case it's loaded in a same-origin subframe. In this
case we should always have access to the main frame I believe.

dmazzoni: you may want to do some tests of these cases with OOPIF to see the
result.

[dmazzoni, 2016-06-30 22:35:14]

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/20001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:73: content::RenderFrame*
render_frame = render_view_->GetMainRenderFrame();
On 2016/06/29 00:31:36, raymes wrote:
> On 2016/06/28 21:59:51, nasko wrote:
> > Are we guaranteed to have a main frame? We avoid making this assumption in
the
> > face of out-of-process iframes, in which case the return value will be
> nullptr.
> > If it is guaranteed to exist, it might be useful to put a comment explaining
> > why.
> 
> Good question and I don't know the answer for sure. There are 2 cases where we
> load the PDF plugin: 
> 1) If a PDF is loaded on a web page, we will load a browser plugin. This could
> be at the top level (most commonly) or in an iframe. I'm not sure how browser
> plugin works with OOPIF and I'm not sure how accessibility information gets
> propagated for iframes with OOPIF so it would depend on those 2 things.

In this case, GetMainFrame() would always work as currently implemented,
because the browser plugin always has a guest WebContents with the PDF
extension as its main frame.

It did expose another bug, though - OutOfProcessInstance wasn't loading
accessibility if it wasn't full-window. Fixed.

I added several new tests, including a PDF in an iframe and a PDF in
an OOPIF.

> 2) In print preview. In this case it's loaded in a same-origin subframe. In
this
> case we should always have access to the main frame I believe.
> 
> dmazzoni: you may want to do some tests of these cases with OOPIF to see the
> result.

Ah! This case was actually wrong, thanks for pointing it out. We had
access to the main frame, yes - but that wasn't the host frame for
the PDF plug-in, so we weren't attaching the PDF's accessibility
tree to anything.

I fixed this by adding an interface to get the RenderFrame from the
instance, rather than the RenderView. Now this case works and I
added a test for Print Preview PDF accessibility too.

[raymes, 2016-07-12 01:36:24]

Thanks! A few more thoughts but otherwise looks good

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:84:
content::IsolateAllSitesForTesting(command_line);
Don't we have site isolation bots that will run all the tests with these flags
enabled? If not, is the preference these days to run all tests with these flags?
Aren't we changing the nature of our test coverage if we do that?

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:89: host_resolver()->AddRule("*",
"127.0.0.1");
How come this is needed?

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:609: // Now enable accessibility
globally, and assert that the
nit: fill 80 chars

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:643: ui::AXTreeUpdate ax_tree =
GetAccessibilityTreeSnapshot(guest_contents);
Hmm, why can't we get the accessibility tree from the outer web contents? (I'm
curious because it seems like WaitForAccessibilityTreeToContainNodeWithName
works on the outer web contents)

https://codereview.chromium.org/2100753003/diff/120001/components/pdf/rendere...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/components/pdf/rendere...
components/pdf/renderer/pepper_pdf_host.cc:240: new
PdfAccessibilityTree(instance->GetRenderFrame()));
There's already a function in RendererPpapiHost (host_) to get the RenderFrame.
I also /believe/ that the RenderFrame can be destroyed before this object, so we
shouldn't rely on the pointer being valid. Instead we should call
GetRenderFrameForInstance and cehck the result each time we need to use it.,

[nasko, 2016-07-12 18:49:07]

This version looks fine to me. Waiting for raymes@ to be happy before stamping
it.

[dmazzoni, 2016-07-13 23:17:12]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[dmazzoni, 2016-07-13 23:17:53]

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:84:
content::IsolateAllSitesForTesting(command_line);
On 2016/07/12 at 01:36:23, raymes wrote:
> Don't we have site isolation bots that will run all the tests with these flags
enabled? If not, is the preference these days to run all tests with these flags?
Aren't we changing the nature of our test coverage if we do that?

My understanding is that when the test is agnostic and you just want to run the
test once with site isolation and once without, you can use those bots.

In this particular case, there are some minor differences to account for, so we
have separate tests.

content::IsolateAllSitesForTesting is used in ~24 files currently, so it doesn't
seem to be deprecated.

Nasko, please correct me if I'm wrong!

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:89: host_resolver()->AddRule("*",
"127.0.0.1");
On 2016/07/12 at 01:36:23, raymes wrote:
> How come this is needed?

This allows us to use a url of "/cross-site/1.com/pdf/test-bookmarks.pdf" for
the iframe, and instead of trying to visit 1.com, it resolves to localhost.

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:609: // Now enable accessibility
globally, and assert that the
On 2016/07/12 at 01:36:23, raymes wrote:
> nit: fill 80 chars

Done

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:643: ui::AXTreeUpdate ax_tree =
GetAccessibilityTreeSnapshot(guest_contents);
On 2016/07/12 at 01:36:23, raymes wrote:
> Hmm, why can't we get the accessibility tree from the outer web contents? (I'm
curious because it seems like WaitForAccessibilityTreeToContainNodeWithName
works on the outer web contents)

It's easy to get a single frame's accessibility tree as an AXTreeUpdate,
which is a pretty simple data structure that's easy to dump in
DumpPdfAccessibilityTree. In order to get the accessibility tree for
the whole page including subframes, we'd need to export a lot more
interfaces in content/public.

I'd be happy to do that if it was necessary to fully test it, but it doesn't
seem important because WaitForAccessibilityTreeToContainNodeWithName already
gives us confidence that the PDF content is reachable from the outermost frame.
The rest of the test asserts that the PDF content is complete and correct.

https://codereview.chromium.org/2100753003/diff/120001/components/pdf/rendere...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/components/pdf/rendere...
components/pdf/renderer/pepper_pdf_host.cc:240: new
PdfAccessibilityTree(instance->GetRenderFrame()));
On 2016/07/12 at 01:36:23, raymes wrote:
> There's already a function in RendererPpapiHost (host_) to get the
RenderFrame. I also /believe/ that the RenderFrame can be destroyed before this
object, so we shouldn't rely on the pointer being valid. Instead we should call
GetRenderFrameForInstance and cehck the result each time we need to use it.,

Got it.

OK, now I'm passing RendererPpapiHost and PP_Instance to the
PdfAccessibilityTree constructor, that should resolve any
potential lifetime issues.

[commit-bot: I haz the power, 2016-07-13 23:18:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 00:35:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 00:35:51]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-07-14 03:15:55]

lgtm with a few questions

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:84:
content::IsolateAllSitesForTesting(command_line);
On 2016/07/13 23:17:53, dmazzoni_ooo_until_7_20 wrote:
> On 2016/07/12 at 01:36:23, raymes wrote:
> > Don't we have site isolation bots that will run all the tests with these
flags
> enabled? If not, is the preference these days to run all tests with these
flags?
> Aren't we changing the nature of our test coverage if we do that?
> 
> My understanding is that when the test is agnostic and you just want to run
the
> test once with site isolation and once without, you can use those bots.
> 
> In this particular case, there are some minor differences to account for, so
we
> have separate tests.
> 
> content::IsolateAllSitesForTesting is used in ~24 files currently, so it
doesn't
> seem to be deprecated.
> 
> Nasko, please correct me if I'm wrong!

Should we only enable it in PdfAccessibilityInOOPIF then? I also defer to
Nasko's advice :)

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:89: host_resolver()->AddRule("*",
"127.0.0.1");
Thanks. This might conflict with
https://codereview.chromium.org/2127383002/diff/140001/chrome/browser/pdf/pdf...
which was just landed, so we should make sure that it still works. Can the rule
be more specific than *?

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:643: ui::AXTreeUpdate ax_tree =
GetAccessibilityTreeSnapshot(guest_contents);
On 2016/07/13 23:17:53, dmazzoni_ooo_until_7_20 wrote:
> On 2016/07/12 at 01:36:23, raymes wrote:
> > Hmm, why can't we get the accessibility tree from the outer web contents?
(I'm
> curious because it seems like WaitForAccessibilityTreeToContainNodeWithName
> works on the outer web contents)
> 
> It's easy to get a single frame's accessibility tree as an AXTreeUpdate,
> which is a pretty simple data structure that's easy to dump in
> DumpPdfAccessibilityTree. In order to get the accessibility tree for
> the whole page including subframes, we'd need to export a lot more
> interfaces in content/public.
> 
> I'd be happy to do that if it was necessary to fully test it, but it doesn't
> seem important because WaitForAccessibilityTreeToContainNodeWithName already
> gives us confidence that the PDF content is reachable from the outermost
frame.
> The rest of the test asserts that the PDF content is complete and correct.

I still don't fully understand. Does
WaitForAccessibilityTreeToContainNodeWithName have access to some of the data
but not all of it (from the outer web contents?). In any case it's not too
important - I trust this is the easiest way to write the test.

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:256:
DCHECK(render_accessibility);
Can we trust this to be non-null?

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:269: DCHECK(render_frame);
And this?

[nasko, 2016-07-14 18:21:44]

LGTM

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:84:
content::IsolateAllSitesForTesting(command_line);
On 2016/07/14 03:15:54, raymes wrote:
> On 2016/07/13 23:17:53, dmazzoni_ooo_until_7_20 wrote:
> > On 2016/07/12 at 01:36:23, raymes wrote:
> > > Don't we have site isolation bots that will run all the tests with these
> flags
> > enabled? If not, is the preference these days to run all tests with these
> flags?
> > Aren't we changing the nature of our test coverage if we do that?
> > 
> > My understanding is that when the test is agnostic and you just want to run
> the
> > test once with site isolation and once without, you can use those bots.
> > 
> > In this particular case, there are some minor differences to account for, so
> we
> > have separate tests.
> > 
> > content::IsolateAllSitesForTesting is used in ~24 files currently, so it
> doesn't
> > seem to be deprecated.
> > 
> > Nasko, please correct me if I'm wrong!
> 
> Should we only enable it in PdfAccessibilityInOOPIF then? I also defer to
> Nasko's advice :)

Long term the goal is to eliminate the FYI bots that run with
--site-per-process, so having tests that explicitly want to test OOPIF behavior
will be good to enable it explicitly.
That said, enabling it on tests that don't require it will not hurt, so I'm fine
either way you choose.

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:89: host_resolver()->AddRule("*",
"127.0.0.1");
On 2016/07/14 03:15:54, raymes wrote:
> Thanks. This might conflict with
>
https://codereview.chromium.org/2127383002/diff/140001/chrome/browser/pdf/pdf...
> which was just landed, so we should make sure that it still works. Can the
rule
> be more specific than *?

We have the * rule all over the place. What is a good reason to keep it more
restricted?

[dmazzoni, 2016-07-14 21:04:41]

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/120001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:643: ui::AXTreeUpdate ax_tree =
GetAccessibilityTreeSnapshot(guest_contents);
On 2016/07/14 at 03:15:54, raymes wrote:
> On 2016/07/13 23:17:53, dmazzoni_ooo_until_7_20 wrote:
> > On 2016/07/12 at 01:36:23, raymes wrote:
> > > Hmm, why can't we get the accessibility tree from the outer web contents?
(I'm
> > curious because it seems like WaitForAccessibilityTreeToContainNodeWithName
> > works on the outer web contents)
> > 
> > It's easy to get a single frame's accessibility tree as an AXTreeUpdate,
> > which is a pretty simple data structure that's easy to dump in
> > DumpPdfAccessibilityTree. In order to get the accessibility tree for
> > the whole page including subframes, we'd need to export a lot more
> > interfaces in content/public.
> > 
> > I'd be happy to do that if it was necessary to fully test it, but it doesn't
> > seem important because WaitForAccessibilityTreeToContainNodeWithName already
> > gives us confidence that the PDF content is reachable from the outermost
frame.
> > The rest of the test asserts that the PDF content is complete and correct.
> 
> I still don't fully understand. Does
WaitForAccessibilityTreeToContainNodeWithName have access to some of the data
but not all of it (from the outer web contents?). In any case it's not too
important - I trust this is the easiest way to write the test.

WaitForAccessibilityTreeToContainNodeWithName is implemented inside
of content and it explores across frame/plugin boundaries - it just only
checks for a single node name, though.

Since the node we're waiting for only appears in the PDF we can be
pretty confident the PDF loaded.

But then GetAccessibilityTreeSnapshot only gets a single frame.
Getting multiple frames would require either some extra processing
to renumber IDs (since IDs are unique only within frames) or
exporting a more complex data structure from content/.

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:256:
DCHECK(render_accessibility);
On 2016/07/14 at 03:15:54, raymes wrote:
> Can we trust this to be non-null?

This is private. I did a null check for RenderAccessibility at the top of each
public function so this should be safe.

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:269: DCHECK(render_frame);
On 2016/07/14 at 03:15:54, raymes wrote:
> And this?

Same, private and will only be called after we know it's
not null.

[dmazzoni, 2016-07-14 21:04:59]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:05:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:15:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:15:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-07-15 18:18:51]

dmazzoni@chromium.org changed reviewers:
+ thestig@chromium.org

[dmazzoni, 2016-07-15 18:18:52]

+thestig for
chrome/browser/printing/print_preview_dialog_controller_browsertest.cc

[Lei Zhang, 2016-07-15 19:26:21]

lgtm

https://codereview.chromium.org/2100753003/diff/160001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:559: static const char*
kExpectedPDFAXTree =
const char kFoo[], otherwise you can potentially write kFoo = nullptr.

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:278: return
render_frame->GetRenderAccessibility();
ternary operator?

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.h (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.h:32: explicit
PdfAccessibilityTree(content::RendererPpapiHost* host,
explicit no longer needed.

[dmazzoni, 2016-07-18 04:28:27]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[dmazzoni, 2016-07-18 04:28:33]

https://codereview.chromium.org/2100753003/diff/160001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:559: static const char*
kExpectedPDFAXTree =
On 2016/07/15 at 19:26:21, Lei Zhang wrote:
> const char kFoo[], otherwise you can potentially write kFoo = nullptr.

Done

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:278: return
render_frame->GetRenderAccessibility();
On 2016/07/15 at 19:26:21, Lei Zhang wrote:
> ternary operator?

Done

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.h (right):

https://codereview.chromium.org/2100753003/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.h:32: explicit
PdfAccessibilityTree(content::RendererPpapiHost* host,
On 2016/07/15 at 19:26:21, Lei Zhang wrote:
> explicit no longer needed.

Done

[commit-bot: I haz the power, 2016-07-18 04:28:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2016-07-18 05:14:19]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-07-18 05:14:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, raymes@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2100753003/#ps200001
(title: "Ready for commit")

[commit-bot: I haz the power, 2016-07-18 05:14:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 06:15:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 06:15:06]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dmazzoni, 2016-07-18 20:31:21]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-07-18 20:31:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, raymes@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2100753003/#ps220001
(title: "Work around cross-platform whitespace difference in print preview
test")

[commit-bot: I haz the power, 2016-07-18 20:32:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 21:45:50]

Description was changed from

==========
Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
==========

to

==========
Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
==========

[commit-bot: I haz the power, 2016-07-18 21:45:51]

Committed patchset #12 (id:220001)

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

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 21:48:34]

Description was changed from

==========
Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724
==========

to

==========
Enable PDF accessibility when RenderFrame's accessibility mode changes.

This does two things: (1) enables accessibility for a PDF plug-in when
the accessibility mode changes for a RenderFrame (previously we only
checked the mode once on creation), and (2) fixes a potential UAF
in PdfAccessibilityTree if the RenderFrame deletes the
RenderAccessibilityImpl, in particular during loading.

BUG=54724

Committed: https://crrev.com/5a0018c9adcdc04c636d382f06e34bd6569ac791
Cr-Commit-Position: refs/heads/master@{#406108}
==========

[commit-bot: I haz the power, 2016-07-18 21:48:35]

Patchset 12 (id:??) landed as
https://crrev.com/5a0018c9adcdc04c636d382f06e34bd6569ac791
Cr-Commit-Position: refs/heads/master@{#406108}