Limit PDF helper extension to print preview only

Limit PDF helper extension to print preview only

These days, the helper extension is only used by the print preview.
So make sure that the helper extension is only activated in print
previews and fall back to the standard MimeHandler otherwise.
When the helper extension is unexpectedly loaded directly instead
of through the mime handler, the extension will fail to initialize.
This is totally desired!

BUG=654280
TEST=See bug report, also confirmed that print preview still works.
R=thestig@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c72f59f36a16c0cb64e0eebfc0999a09250740f0
Cr-Commit-Position: refs/heads/master@{#425280}

[robwu, 2016-10-09 16:31:28]

Description was changed from

==========
Limit PDF helper extension to print preview only

These days, the helper extension is only used by the print preview.
So make sure that the helper extension is only activated in print
previews and fall back to the standard MimeHandler otherwise.
When the helper extension is unexpectedly loaded directly instead
of through the mime handler, the extension will fail to initialize.
This is totally desired!

BUG=654280
TEST=See bug report, also confirmed that print preview still works.
R=thestig@chromium.org
==========

to

==========
Limit PDF helper extension to print preview only

These days, the helper extension is only used by the print preview.
So make sure that the helper extension is only activated in print
previews and fall back to the standard MimeHandler otherwise.
When the helper extension is unexpectedly loaded directly instead
of through the mime handler, the extension will fail to initialize.
This is totally desired!

BUG=654280
TEST=See bug report, also confirmed that print preview still works.
R=thestig@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[robwu, 2016-10-09 16:31:31]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 16:31:38]

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-10-09 17:10:16]

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

[commit-bot: I haz the power, 2016-10-09 17:10:16]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-10-10 21:15:42]

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

[Lei Zhang, 2016-10-10 21:15:42]

I defer to raymes on this one.

[raymes, 2016-10-11 06:24:44]

Thanks for this! This lg but I think we should also prevent any redirects
occurring inside the plugin (as is being discussed in
https://codereview.chromium.org/2407683002/). That would also fix the bug.

Could you add a test that verifies the extension doesn't load when embedded on a
random page to pdf_extension_test.cc? If not I can probably look at adding
something.

https://codereview.chromium.org/2407713002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/browser_api.js:209: if (location.search &&
location.ancestorOrigins[0] === 'chrome://print')
Do we need the window.location.search check too?

[robwu, 2016-10-11 21:05:37]

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

[robwu, 2016-10-11 21:05:38]

+Mustafa for review of content/public/test.
I moved ConsoleObserverDelegate to browser_test_utils so I can use it in
browser_tests. Since you moved it before I figured that you're probably the best
reviewer here.


On 2016/10/11 06:24:44, raymes wrote:
> Thanks for this! This lg but I think we should also prevent any redirects
> occurring inside the plugin (as is being discussed in
> https://codereview.chromium.org/2407683002/). That would also fix the bug.

This is already handled by another bug. Also, it would fix the particular
example that I showed but not the root cause (which is fixed here).

> Could you add a test that verifies the extension doesn't load when embedded on
a
> random page to pdf_extension_test.cc? If not I can probably look at adding
> something.

I've added a test, PTAL!

https://codereview.chromium.org/2407713002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/browser_api.js:209: if (location.search &&
location.ancestorOrigins[0] === 'chrome://print')
On 2016/10/11 06:24:44, raymes wrote:
> Do we need the window.location.search check too?

Yes. When loaded inside the plugin location.ancestorOrigins.length === 0, which
is indistinguishable from being loaded in the top-level frame.

[robwu, 2016-10-11 21:09:29]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[robwu, 2016-10-11 21:09:39]

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

[robwu, 2016-10-11 21:09:39]

rob@robwu.nl changed required reviewers:
+ nasko@chromium.org, raymes@chromium.org

[robwu, 2016-10-11 21:09:40]

-meacer
+nasko

Nasko, can you review the mechanical move in content/public/test? See my
previous message.

Mustafa is not an OWNER so asking for review doesn't help here.

[commit-bot: I haz the power, 2016-10-11 21:09:57]

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-10-11 21:20:17]

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

[commit-bot: I haz the power, 2016-10-11 21:20:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[nasko, 2016-10-11 21:26:33]

content/ LGTM and a couple of nits.

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:467: // Ensure that the PDF component
extension cannot directly be loaded.
nit: s/cannot directly be loaded/cannot be loaded directly/.

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:474: web_contents,
nit: Shouldn't this be 4 spaces indent? git cl format should help ensure proper
formatting.

[robwu, 2016-10-11 22:15:46]

rob@robwu.nl changed reviewers:
- meacer@chromium.org

[robwu, 2016-10-11 22:15:46]

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:467: // Ensure that the PDF component
extension cannot directly be loaded.
On 2016/10/11 21:26:33, nasko (slow) wrote:
> nit: s/cannot directly be loaded/cannot be loaded directly/.

Done.

https://codereview.chromium.org/2407713002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:474: web_contents,
On 2016/10/11 21:26:33, nasko (slow) wrote:
> nit: Shouldn't this be 4 spaces indent? git cl format should help ensure
proper
> formatting.

Yes, four it is. Done!

[robwu, 2016-10-11 22:16:00]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 22:16:18]

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-10-11 22:30:53]

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

[commit-bot: I haz the power, 2016-10-11 22:30:54]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[raymes, 2016-10-11 23:32:34]

On 2016/10/11 21:05:38, robwu wrote:
> +Mustafa for review of content/public/test.
> I moved ConsoleObserverDelegate to browser_test_utils so I can use it in
> browser_tests. Since you moved it before I figured that you're probably the
best
> reviewer here.
> 
> 
> On 2016/10/11 06:24:44, raymes wrote:
> > Thanks for this! This lg but I think we should also prevent any redirects
> > occurring inside the plugin (as is being discussed in
> > https://codereview.chromium.org/2407683002/). That would also fix the bug.
> 
> This is already handled by another bug. Also, it would fix the particular
> example that I showed but not the root cause (which is fixed here).

Actually I think the root cause of both bugs is the same - it's the fact that
redirects inside the plugin make the security checks invalid. Embedding the
plugin directly may enable this to happen more easily (and we should probably
lock that down) but I think it's the same underlying security issue. I might be
missing something though.

[robwu, 2016-10-11 23:40:03]

On 2016/10/11 23:32:34, raymes wrote:
> Actually I think the root cause of both bugs is the same - it's the fact that
> redirects inside the plugin make the security checks invalid. Embedding the
> plugin directly may enable this to happen more easily (and we should probably
> lock that down) but I think it's the same underlying security issue. I might
be
> missing something though.

The bug report showed two examples, abusing redirects was only one of them.

[raymes, 2016-10-11 23:50:29]

On 2016/10/11 23:40:03, robwu wrote:
> On 2016/10/11 23:32:34, raymes wrote:
> > Actually I think the root cause of both bugs is the same - it's the fact
that
> > redirects inside the plugin make the security checks invalid. Embedding the
> > plugin directly may enable this to happen more easily (and we should
probably
> > lock that down) but I think it's the same underlying security issue. I might
> be
> > missing something though.
> 
> The bug report showed two examples, abusing redirects was only one of them.

Which examples do you mean?

[robwu, 2016-10-11 23:57:28]

On 2016/10/11 23:50:29, raymes wrote:
> On 2016/10/11 23:40:03, robwu wrote:
> > On 2016/10/11 23:32:34, raymes wrote:
> > > Actually I think the root cause of both bugs is the same - it's the fact
> that
> > > redirects inside the plugin make the security checks invalid. Embedding
the
> > > plugin directly may enable this to happen more easily (and we should
> probably
> > > lock that down) but I think it's the same underlying security issue. I
might
> > be
> > > missing something though.
> > 
> > The bug report showed two examples, abusing redirects was only one of them.
> 
> Which examples do you mean?

See the bullet points immediately below "including but not limited to" in the
bug linked from this CL description.

[raymes, 2016-10-11 23:59:58]

On 2016/10/11 23:57:28, robwu wrote:
> On 2016/10/11 23:50:29, raymes wrote:
> > On 2016/10/11 23:40:03, robwu wrote:
> > > On 2016/10/11 23:32:34, raymes wrote:
> > > > Actually I think the root cause of both bugs is the same - it's the fact
> > that
> > > > redirects inside the plugin make the security checks invalid. Embedding
> the
> > > > plugin directly may enable this to happen more easily (and we should
> > probably
> > > > lock that down) but I think it's the same underlying security issue. I
> might
> > > be
> > > > missing something though.
> > > 
> > > The bug report showed two examples, abusing redirects was only one of
them.
> > 
> > Which examples do you mean?
> 
> See the bullet points immediately below "including but not limited to" in the
> bug linked from this CL description.

Ah yes, I understand now, thanks for the pointer.

[raymes, 2016-10-12 02:08:51]

raymes@chromium.org changed required reviewers:
- raymes@chromium.org

[raymes, 2016-10-12 02:08:52]

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:481: console_delegate->Wait();
It could be good to verify that the plugin element isn't even created. Take a
look at EnsureInternalPluginDisabled, I think it should just be a matter of
checking something similar.

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:210: if (location.ancestorOrigins[0]
!== 'chrome://print') {
I think it could be good to rework these 2 checks. We should actually only ever
use the createBrowserApiForStandaloneExtension API when we're in print preview
(we could rename it to createBrowserApiForPrintPreview). We actually load the
pdf viewer in the context of chrome://print. So I think this can just boil down
to: 
if (document.location.origin == "chrome://print") 
  createBrowserApiForPrintPreview()

Similarly, in pdf.js, we should change this.isPrintPreview_ to check the same
thing. Could you add that to this CL?


In the plugin, we should do something similar (though this can go in a different
CL):
1) Change the IsPrintPreview() check to use the document URL
2) When we load a new pdf, to check that the URL being loaded is a
chrome://print URL.

[robwu, 2016-10-12 11:33:28]

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:481: console_delegate->Wait();
On 2016/10/12 02:08:52, raymes wrote:
> It could be good to verify that the plugin element isn't even created. Take a
> look at EnsureInternalPluginDisabled, I think it should just be a matter of
> checking something similar.

The actual plugin is created asynchronously. I added the following:

  bool plugin_loaded = false;                                                  
  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(                            
      web_contents,                                                            
      "var plugin_selector = 'embed[type=\"application/x-google-chrome-pdf\"]';"
      "var plugin_loaded = document.querySelector(plugin_selector) !== null;"  
      "window.domAutomationController.send(plugin_loaded);",                   
      &plugin_loaded));                                                        
  ASSERT_FALSE(plugin_loaded);                                                 

... but this did not fail without my patch, so it is useless.
I think that checking for the expected error message is sufficient. After all,
even if the plugin were to be loaded somehow, it cannot be scripted by the
parent frame.

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:210: if (location.ancestorOrigins[0]
!== 'chrome://print') {
On 2016/10/12 02:08:52, raymes wrote:
> I think it could be good to rework these 2 checks. We should actually only
ever
> use the createBrowserApiForStandaloneExtension API when we're in print preview
> (we could rename it to createBrowserApiForPrintPreview). We actually load the
> pdf viewer in the context of chrome://print. So I think this can just boil
down
> to: 
> if (document.location.origin == "chrome://print") 
>   createBrowserApiForPrintPreview()
> 
> Similarly, in pdf.js, we should change this.isPrintPreview_ to check the same
> thing. Could you add that to this CL?
> 
> 
> In the plugin, we should do something similar (though this can go in a
different
> CL):
> 1) Change the IsPrintPreview() check to use the document URL
> 2) When we load a new pdf, to check that the URL being loaded is a
> chrome://print URL.

Are you sure about the claim that this snippet runs in the context of
chrome://print? The source seems to show that chrome://print embeds the helper
extension in an iframe: 
https://chromium.googlesource.com/chromium/src/+blame/a223560fd493a9f25d23a33...
https://chromium.googlesource.com/chromium/src/+blame/a41883ee67ef28c0bd69aea...

[raymes, 2016-10-13 00:43:22]

Thanks!

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:481: console_delegate->Wait();
On 2016/10/12 11:33:28, robwu wrote:
> On 2016/10/12 02:08:52, raymes wrote:
> > It could be good to verify that the plugin element isn't even created. Take
a
> > look at EnsureInternalPluginDisabled, I think it should just be a matter of
> > checking something similar.
> 
> The actual plugin is created asynchronously. I added the following:
> 
>   bool plugin_loaded = false;                                                 

>   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(                           

>       web_contents,                                                           

>       "var plugin_selector =
'embed[type=\"application/x-google-chrome-pdf\"]';"
>       "var plugin_loaded = document.querySelector(plugin_selector) !== null;" 

>       "window.domAutomationController.send(plugin_loaded);",                  

>       &plugin_loaded));                                                       

>   ASSERT_FALSE(plugin_loaded);                                                

> 
> ... but this did not fail without my patch, so it is useless.
> I think that checking for the expected error message is sufficient. After all,
> even if the plugin were to be loaded somehow, it cannot be scripted by the
> parent frame.

Ah right. Ok - yeah testing the absence of success is hard.

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:151: function
createBrowserApiForMimeHandlerView() {
With my suggestion below, we won't throw an exception there - anymore. Instead
we will get 
"Unchecked runtime.lastError while running mimeHandlerPrivate.getStreamInfo:
Streams are only available from a mime handler view guest."

I think that exception is meaningful enough, so we can just test for the
presence of that.

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:210: if (location.ancestorOrigins[0]
!== 'chrome://print') {
On 2016/10/12 11:33:28, robwu wrote:
> On 2016/10/12 02:08:52, raymes wrote:
> > I think it could be good to rework these 2 checks. We should actually only
> ever
> > use the createBrowserApiForStandaloneExtension API when we're in print
preview
> > (we could rename it to createBrowserApiForPrintPreview). We actually load
the
> > pdf viewer in the context of chrome://print. So I think this can just boil
> down
> > to: 
> > if (document.location.origin == "chrome://print") 
> >   createBrowserApiForPrintPreview()
> > 
> > Similarly, in pdf.js, we should change this.isPrintPreview_ to check the
same
> > thing. Could you add that to this CL?
> > 
> > 
> > In the plugin, we should do something similar (though this can go in a
> different
> > CL):
> > 1) Change the IsPrintPreview() check to use the document URL
> > 2) When we load a new pdf, to check that the URL being loaded is a
> > chrome://print URL.
> 
> Are you sure about the claim that this snippet runs in the context of
> chrome://print? The source seems to show that chrome://print embeds the helper
> extension in an iframe: 
>
https://chromium.googlesource.com/chromium/src/+blame/a223560fd493a9f25d23a33...
>
https://chromium.googlesource.com/chromium/src/+blame/a41883ee67ef28c0bd69aea...

Yes I'm fairly sure. PDFCreateOutOfProcessPlugin loads pdf_preview.html which is
loaded from chrome://print. You will be able to test it easily though, if my
assumption is false then print preview should be broken with the check I
suggested.

[robwu, 2016-10-13 23:29:30]

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/pdf/browser_api.js (right):

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:151: function
createBrowserApiForMimeHandlerView() {
On 2016/10/13 00:43:22, raymes wrote:
> With my suggestion below, we won't throw an exception there - anymore. Instead
> we will get 
> "Unchecked runtime.lastError while running mimeHandlerPrivate.getStreamInfo:
> Streams are only available from a mime handler view guest."
> 
> I think that exception is meaningful enough, so we can just test for the
> presence of that.

Done.

https://codereview.chromium.org/2407713002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/browser_api.js:210: if (location.ancestorOrigins[0]
!== 'chrome://print') {
On 2016/10/13 00:43:22, raymes wrote:
> On 2016/10/12 11:33:28, robwu wrote:
> > On 2016/10/12 02:08:52, raymes wrote:
> > > I think it could be good to rework these 2 checks. We should actually only
> > ever
> > > use the createBrowserApiForStandaloneExtension API when we're in print
> preview
> > > (we could rename it to createBrowserApiForPrintPreview). We actually load
> the
> > > pdf viewer in the context of chrome://print. So I think this can just boil
> > down
> > > to: 
> > > if (document.location.origin == "chrome://print") 
> > >   createBrowserApiForPrintPreview()
> > > 
> > > Similarly, in pdf.js, we should change this.isPrintPreview_ to check the
> same
> > > thing. Could you add that to this CL?
> > > 
> > > 
> > > In the plugin, we should do something similar (though this can go in a
> > different
> > > CL):
> > > 1) Change the IsPrintPreview() check to use the document URL
> > > 2) When we load a new pdf, to check that the URL being loaded is a
> > > chrome://print URL.
> > 
> > Are you sure about the claim that this snippet runs in the context of
> > chrome://print? The source seems to show that chrome://print embeds the
helper
> > extension in an iframe: 
> >
>
https://chromium.googlesource.com/chromium/src/+blame/a223560fd493a9f25d23a33...
> >
>
https://chromium.googlesource.com/chromium/src/+blame/a41883ee67ef28c0bd69aea...
> 
> Yes I'm fairly sure. PDFCreateOutOfProcessPlugin loads pdf_preview.html which
is
> loaded from chrome://print. You will be able to test it easily though, if my
> assumption is false then print preview should be broken with the check I
> suggested.

Done.

[robwu, 2016-10-13 23:29:34]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 23:30:02]

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

[raymes, 2016-10-13 23:36:01]

lgtm

[commit-bot: I haz the power, 2016-10-14 00:26:30]

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

[commit-bot: I haz the power, 2016-10-14 00:26:31]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-10-14 09:28:20]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-10-14 09:28:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2407713002/#ps60001
(title: "Use location.origin instead of location.ancestorOrigins.")

[commit-bot: I haz the power, 2016-10-14 09:28:39]

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-10-14 09:35:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-14 09:38:33]

Description was changed from

==========
Limit PDF helper extension to print preview only

These days, the helper extension is only used by the print preview.
So make sure that the helper extension is only activated in print
previews and fall back to the standard MimeHandler otherwise.
When the helper extension is unexpectedly loaded directly instead
of through the mime handler, the extension will fail to initialize.
This is totally desired!

BUG=654280
TEST=See bug report, also confirmed that print preview still works.
R=thestig@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Limit PDF helper extension to print preview only

These days, the helper extension is only used by the print preview.
So make sure that the helper extension is only activated in print
previews and fall back to the standard MimeHandler otherwise.
When the helper extension is unexpectedly loaded directly instead
of through the mime handler, the extension will fail to initialize.
This is totally desired!

BUG=654280
TEST=See bug report, also confirmed that print preview still works.
R=thestig@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c72f59f36a16c0cb64e0eebfc0999a09250740f0
Cr-Commit-Position: refs/heads/master@{#425280}
==========

[commit-bot: I haz the power, 2016-10-14 09:38:34]

Patchset 4 (id:??) landed as
https://crrev.com/c72f59f36a16c0cb64e0eebfc0999a09250740f0
Cr-Commit-Position: refs/heads/master@{#425280}