Add UI for RuntimeErrors in the ErrorConsole

Follows https://codereview.chromium.org/22938005/

Add UI elements for extension runtime (javascript) errors on the chrome:extensions page.  Include a stack trace and, if possible, links to the source code.

TBR=finnur@chromium.org
BUG=21734
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221824

[Devlin, 2013-09-04 19:37:30]

Since Manifest UI should be going through soon, please take a look.

Cheers =)

[Yoyo Zhou, 2013-09-04 23:43:39]

LGTM, but I didn't review the js.

https://codereview.chromium.org/23624002/diff/11001/extensions/browser/file_h...
File extensions/browser/file_highlighter.h (right):

https://codereview.chromium.org/23624002/diff/11001/extensions/browser/file_h...
extensions/browser/file_highlighter.h:41: void
HighlightDictionary(base::DictionaryValue* dict) const;
This name could be better, like SetHighlightedRegions.

https://codereview.chromium.org/23624002/diff/11001/extensions/browser/file_h...
extensions/browser/file_highlighter.h:43: // The keys used in highlighting a
dictionary.
These needn't be declared in the header file if they're not used anywhere else.

[not at google - send to devlin, 2013-09-05 00:37:15]

got a small way through the JS/HTML/CSS before the end of the day, but fairly
superficially. will need to do a bunch of background reading to see how this
stuff works.

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.html (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.html:15: <div
class="extension-error-detailed-wrapper">
"detailed" seems deliberate here but it looks weird, maybe comments would help
here... but I'm not that familiar with the idiom of comments in webui

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:48: if (error.stackTrace
|| error.contextUrl) {
A system that I think would make more sense here is to pass in the template name
as a constructor parameter (you'd need to make extension-error-simple-wrapper a
template).

Then rather than isDetailed, just change what you generate based on the presence
of a stack trace or a context url.

there is a lot of if-this and if-that which seems unnecessarily complicated.

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:114: * @param {?number}
line An optional line number of the resource.
s/number/line

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_list.js:323: } else if
(extension.installWarnings) {
I'm puzzled by the else here. Why can't this be:

if (extension.manifestErrors) {
  ... manifest error stuff
}

if (extension.runtimeErrors) {
  ... runtime error stuff
}

if (extension.installWarnings) {
  ... install warning stuff
}

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/extensions/extensions.html:159: <div
class="runtime-errors-wrapper" hidden></div>
why adding the -wrapper part? seems to be an implementation detail of how these
are populated?

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:86: DCHECK_EQ(1u,
args->GetSize());
what was wrong with the CHECK

[Devlin, 2013-09-05 17:53:55]

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_error.html (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.html:15: <div
class="extension-error-detailed-wrapper">
On 2013/09/05 00:37:16, kalman wrote:
> "detailed" seems deliberate here but it looks weird, maybe comments would help
> here... but I'm not that familiar with the idiom of comments in webui

I don't usually see comments in the HTML, but we can add them if they're
necessary.  Does the addition of simple-wrapper here make it clear enough, or
would more explanation be better?

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_error.js (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:48: if (error.stackTrace
|| error.contextUrl) {
On 2013/09/05 00:37:16, kalman wrote:
> A system that I think would make more sense here is to pass in the template
name
> as a constructor parameter (you'd need to make extension-error-simple-wrapper
a
> template).
> 
> Then rather than isDetailed, just change what you generate based on the
presence
> of a stack trace or a context url.
> 
> there is a lot of if-this and if-that which seems unnecessarily complicated.

Helps a bit, but we still have to check for where to apply the metadata (either
to the top-level node or to the summary). But it's probably cleaner overall.

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:114: * @param {?number}
line An optional line number of the resource.
On 2013/09/05 00:37:16, kalman wrote:
> s/number/line

I thought it was supposed to follow
@param {type} name Description...

Are there exceptions?

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_list.js (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_list.js:323: } else if
(extension.installWarnings) {
On 2013/09/05 00:37:16, kalman wrote:
> I'm puzzled by the else here. Why can't this be:
> 
> if (extension.manifestErrors) {
>   ... manifest error stuff
> }
> 
> if (extension.runtimeErrors) {
>   ... runtime error stuff
> }
> 
> if (extension.installWarnings) {
>   ... install warning stuff
> }

It can be. I had it like that for two reasons:
1. It makes the logic for possible entities more clear: if we have
runtime/manifest errors, we do not have install warnings.
2. It adds an extra layer to make sure we don't show something silly if our
assumptions are ever wrong (even though they shouldn't be).

But, of course, if the block is more puzzling than the alternative, clearly #1
failed. So changed. :)

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extensions.html:159: <div
class="runtime-errors-wrapper" hidden></div>
On 2013/09/05 00:37:16, kalman wrote:
> why adding the -wrapper part? seems to be an implementation detail of how
these
> are populated?

True. Removed.

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/ui/...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:86: DCHECK_EQ(1u,
args->GetSize());
On 2013/09/05 00:37:16, kalman wrote:
> what was wrong with the CHECK

I thought a DCHECK would be more consistent, since everywhere else we have
NOTREACHEDs, and I figured it better to fail gracefully in release builds. But I
can change it, if CHECK() is preferred?

https://chromiumcodereview.appspot.com/23624002/diff/11001/extensions/browser...
File extensions/browser/file_highlighter.h (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/extensions/browser...
extensions/browser/file_highlighter.h:41: void
HighlightDictionary(base::DictionaryValue* dict) const;
On 2013/09/04 23:43:39, Yoyo Zhou wrote:
> This name could be better, like SetHighlightedRegions.

Done.

https://chromiumcodereview.appspot.com/23624002/diff/11001/extensions/browser...
extensions/browser/file_highlighter.h:43: // The keys used in highlighting a
dictionary.
On 2013/09/04 23:43:39, Yoyo Zhou wrote:
> These needn't be declared in the header file if they're not used anywhere
else.

Done.

[not at google - send to devlin, 2013-09-06 17:05:19]

Looked at the rest.

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_error.js (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:114: * @param {?number}
line An optional line number of the resource.
On 2013/09/05 17:53:55, D Cronin wrote:
> On 2013/09/05 00:37:16, kalman wrote:
> > s/number/line
> 
> I thought it was supposed to follow
> @param {type} name Description...
> 
> Are there exceptions?

Oops I totally misread that.

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_list.js (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/res...
chrome/browser/resources/extensions/extension_list.js:323: } else if
(extension.installWarnings) {
On 2013/09/05 17:53:55, D Cronin wrote:
> On 2013/09/05 00:37:16, kalman wrote:
> > I'm puzzled by the else here. Why can't this be:
> > 
> > if (extension.manifestErrors) {
> >   ... manifest error stuff
> > }
> > 
> > if (extension.runtimeErrors) {
> >   ... runtime error stuff
> > }
> > 
> > if (extension.installWarnings) {
> >   ... install warning stuff
> > }
> 
> It can be. I had it like that for two reasons:
> 1. It makes the logic for possible entities more clear: if we have
> runtime/manifest errors, we do not have install warnings.
> 2. It adds an extra layer to make sure we don't show something silly if our
> assumptions are ever wrong (even though they shouldn't be).
> 
> But, of course, if the block is more puzzling than the alternative, clearly #1
> failed. So changed. :)

Yeah - it would be simpler to just render what we're told to. Let the C++ code
(which is actually testable) enforce consistency.

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://chromiumcodereview.appspot.com/23624002/diff/11001/chrome/browser/ui/...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:86: DCHECK_EQ(1u,
args->GetSize());
On 2013/09/05 17:53:55, D Cronin wrote:
> On 2013/09/05 00:37:16, kalman wrote:
> > what was wrong with the CHECK
> 
> I thought a DCHECK would be more consistent, since everywhere else we have
> NOTREACHEDs, and I figured it better to fail gracefully in release builds. But
I
> can change it, if CHECK() is preferred?

We will pick it up in canary channel if there's a bug here :) it'd be a hard bug
if the JS is calling us with the wrong number of arguments. CHECK is almost
always better than DCHECK IMO.

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_error.css (right):

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.css:23:
.extension-error-detailed-wrapper {
alphabetise?

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_error.js (right):

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:91:
this.querySelector('summary').appendChild(metadata);
You could do this in a semi-template-agnostic way (less magic would be to
declare where the metadata should go, but that seems like overkill at the
moment):

// The template may specify a <summary> to put template metadata in.
// If not, just append it to the top-level template element.
var metadataContainer = this.querySelector('summary') || this;
metadataContainer.appendChild(metadata);

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:100: }
instead:

var errorDetails = this.querySelector('.extension-error-details');
if (this.error_.contextUrl)
  errorDetails.appendChild(this.getContextNode_());
if (this.error_.stackTrace)
  errorDetails.appendChild(this.getStackNode_());

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:142: var dictionary = {
'extensionId': this.error_.extensionId,
s/dictionary/requestFileSourceArgs?

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:145: if (relativeUrl ===
'manifest.json') {
just ==. No point ===ing for a comparison to a non-empty string.

That said can't you get the C++ to figure all of this out? Why does it need to
be at the JS layer?

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:156:
this.error_.stackTrace[0].lineNumber : 0;
mmmm nested ternary-if confusing.

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:179: this.extensionUrl_);
I believe that innerText is deprecated in favour of textContent these days; a
fair number of cases in this file where you should change that.

Casual top-google-result searching yields:
http://kellegous.com/j/2013/02/27/innertext-vs-textcontent/

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:183: linkNode.target =
'_blank';
I don't understand why the <a>s get hrefs here, while the isExtensionUrl case
doesn't. Could you explain?

https://chromiumcodereview.appspot.com/23624002/diff/22001/chrome/browser/res...
chrome/browser/resources/extensions/extension_error.js:208: description += ' ('
+ functionName + ')';
small point, but + concatenation is inefficient; better to append to an array
then use .join('') on it.

[Devlin, 2013-09-06 18:18:02]

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/11001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:86: DCHECK_EQ(1u,
args->GetSize());
On 2013/09/06 17:05:19, kalman wrote:
> On 2013/09/05 17:53:55, D Cronin wrote:
> > On 2013/09/05 00:37:16, kalman wrote:
> > > what was wrong with the CHECK
> > 
> > I thought a DCHECK would be more consistent, since everywhere else we have
> > NOTREACHEDs, and I figured it better to fail gracefully in release builds.
But
> I
> > can change it, if CHECK() is preferred?
> 
> We will pick it up in canary channel if there's a bug here :) it'd be a hard
bug
> if the JS is calling us with the wrong number of arguments. CHECK is almost
> always better than DCHECK IMO.

CHECK it is.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.css:23:
.extension-error-detailed-wrapper {
On 2013/09/06 17:05:19, kalman wrote:
> alphabetise?

Done.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:91:
this.querySelector('summary').appendChild(metadata);
On 2013/09/06 17:05:19, kalman wrote:
> You could do this in a semi-template-agnostic way (less magic would be to
> declare where the metadata should go, but that seems like overkill at the
> moment):
> 
> // The template may specify a <summary> to put template metadata in.
> // If not, just append it to the top-level template element.
> var metadataContainer = this.querySelector('summary') || this;
> metadataContainer.appendChild(metadata);

Oooh, that's much cleaner. Thanks :)

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:100: }
On 2013/09/06 17:05:19, kalman wrote:
> instead:
> 
> var errorDetails = this.querySelector('.extension-error-details');
> if (this.error_.contextUrl)
>   errorDetails.appendChild(this.getContextNode_());
> if (this.error_.stackTrace)
>   errorDetails.appendChild(this.getStackNode_());

Done (s/errorDetails/detailsNode to keep consistent with most others).

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:142: var dictionary = {
'extensionId': this.error_.extensionId,
On 2013/09/06 17:05:19, kalman wrote:
> s/dictionary/requestFileSourceArgs?

Done.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:145: if (relativeUrl ===
'manifest.json') {
On 2013/09/06 17:05:19, kalman wrote:
> just ==. No point ===ing for a comparison to a non-empty string.
> 
> That said can't you get the C++ to figure all of this out? Why does it need to
> be at the JS layer?

s/===/==.  We have to do the check on the JS side because we change the
arguments depending on if it's a manifest file or not. But I suppose the
fileType property is redundant, so removed that.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:156:
this.error_.stackTrace[0].lineNumber : 0;
On 2013/09/06 17:05:19, kalman wrote:
> mmmm nested ternary-if confusing.

Yeah, I was afraid of that (hence the comment)... but an if-else chain is
definitely more readable. Done.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:179: this.extensionUrl_);
On 2013/09/06 17:05:19, kalman wrote:
> I believe that innerText is deprecated in favour of textContent these days; a
> fair number of cases in this file where you should change that.
> 
> Casual top-google-result searching yields:
> http://kellegous.com/j/2013/02/27/innertext-vs-textcontent/

Hadn't heard that before. Good to know. Done.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:183: linkNode.target =
'_blank';
On 2013/09/06 17:05:19, kalman wrote:
> I don't understand why the <a>s get hrefs here, while the isExtensionUrl case
> doesn't. Could you explain?

I didn't actually realize we could navigate directly to an extension url (e.g.,
a background page).  Apparently, we can (even if doing so isn't very exciting). 
Changed.

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:208: description += ' ('
+ functionName + ')';
On 2013/09/06 17:05:19, kalman wrote:
> small point, but + concatenation is inefficient; better to append to an array
> then use .join('') on it.

Is it, though?
http://stackoverflow.com/questions/7299010/why-is-string-concatenation-faster...
http://jsperf.com/string-connect-vs-array-join
http://jsperf.com/join-concat/2

:)

[not at google - send to devlin, 2013-09-06 19:00:15]

lgtm

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/23624002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:208: description += ' ('
+ functionName + ')';
On 2013/09/06 18:18:02, D Cronin wrote:
> On 2013/09/06 17:05:19, kalman wrote:
> > small point, but + concatenation is inefficient; better to append to an
array
> > then use .join('') on it.
> 
> Is it, though?
>
http://stackoverflow.com/questions/7299010/why-is-string-concatenation-faster...
> http://jsperf.com/string-connect-vs-array-join
> http://jsperf.com/join-concat/2
> 
> :)

Heh ok. So in Python it makes a huge, huge difference, and *that* I can back up
with data :)

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:156:
requestFileSourceArgs.lineNumber = 0;
ok now that I can read this properly - why this? 0 seems magical; just not
setting it seems predicatable? Is there some reason that it needs to be set to
0?

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:251:
'extensionErrorsRuntimeErrors';
instead of injecting this 'type' flag, inject the title directly. i.e. make
title the constructor parameter.

[Devlin, 2013-09-06 20:56:29]

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:156:
requestFileSourceArgs.lineNumber = 0;
On 2013/09/06 19:00:15, kalman wrote:
> ok now that I can read this properly - why this? 0 seems magical; just not
> setting it seems predicatable? Is there some reason that it needs to be set to
> 0?

0 will result in no line being highlighted for the source file, but that's logic
better suited on the C++ side.  Removed the else.

https://codereview.chromium.org/23624002/diff/51001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_error.js:251:
'extensionErrorsRuntimeErrors';
On 2013/09/06 19:00:15, kalman wrote:
> instead of injecting this 'type' flag, inject the title directly. i.e. make
> title the constructor parameter.

Done.

[Devlin, 2013-09-06 21:38:27]

+finnur TBR for webui (code reviewed by yoz@, but need an owner)

[commit-bot: I haz the power, 2013-09-06 21:38:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/23624002/6...

[commit-bot: I haz the power, 2013-09-07 00:08:29]

Change committed as 221824

[Finnur, 2013-09-09 09:15:55]

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:108:
base::FilePath path = extension->path().Append(path_suffix);
In the event of a compromised caller (sending in values of |args| we don't
normally expect), can this be used to read any manifest.json on disk)? For
example, if I pass in a suffix of:

../<other extension id>/manifest.json

Will I be able to read the contents of another extension's json file?

In other words, don't you need to check that extension->path().IsParent(path) or
use FilePath::AppendRelativePath() instead of Append?

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:132: contents =
new std::string;  // Owned by GetManifestFileCallback(    )
It looks like this leaks as I don't really see this owned and deleted by
GetManifestFileCallback (it is passed in to the highlighter as a const ref where
it is copied but nobody cleans it up).

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:143: contents =
new std::string;  // Owned by GetSourceFileCallback()
Same problem here: GetSourceFileCallback passes this to the highlighter as a
const ref but does nothing more with it.

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
File extensions/browser/file_highlighter.cc (right):

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
extensions/browser/file_highlighter.cc:207: start_ = contents_.find('\n',
start_) + 1;
The +1 makes the check for npos on line 213 invalid, no? Have you tried what
happens if the file has no linebreaks?

[Finnur, 2013-09-09 09:15:55]

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:108:
base::FilePath path = extension->path().Append(path_suffix);
In the event of a compromised caller (sending in values of |args| we don't
normally expect), can this be used to read any manifest.json on disk)? For
example, if I pass in a suffix of:

../<other extension id>/manifest.json

Will I be able to read the contents of another extension's json file?

In other words, don't you need to check that extension->path().IsParent(path) or
use FilePath::AppendRelativePath() instead of Append?

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:132: contents =
new std::string;  // Owned by GetManifestFileCallback(    )
It looks like this leaks as I don't really see this owned and deleted by
GetManifestFileCallback (it is passed in to the highlighter as a const ref where
it is copied but nobody cleans it up).

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:143: contents =
new std::string;  // Owned by GetSourceFileCallback()
Same problem here: GetSourceFileCallback passes this to the highlighter as a
const ref but does nothing more with it.

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
File extensions/browser/file_highlighter.cc (right):

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
extensions/browser/file_highlighter.cc:207: start_ = contents_.find('\n',
start_) + 1;
The +1 makes the check for npos on line 213 invalid, no? Have you tried what
happens if the file has no linebreaks?

[Devlin, 2013-09-09 16:56:02]

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:108:
base::FilePath path = extension->path().Append(path_suffix);
On 2013/09/09 09:15:56, Finnur wrote:
> In the event of a compromised caller (sending in values of |args| we don't
> normally expect), can this be used to read any manifest.json on disk)? For
> example, if I pass in a suffix of:
> 
> ../<other extension id>/manifest.json
> 
> Will I be able to read the contents of another extension's json file?
> 
> In other words, don't you need to check that extension->path().IsParent(path)
or
> use FilePath::AppendRelativePath() instead of Append?

It's possible that, if the caller is compromised, you could see any other file,
yes.  But, is this really a concern?  This won't expose anything that isn't
already in plaintext on the machine -
$ vim
~/.config/google-chrome/<profile>/Extensions/<extension_id>/<version>/manifest.json
will give you the same information, significantly easier...

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:132: contents =
new std::string;  // Owned by GetManifestFileCallback(    )
On 2013/09/09 09:15:56, Finnur wrote:
> It looks like this leaks as I don't really see this owned and deleted by
> GetManifestFileCallback (it is passed in to the highlighter as a const ref
where
> it is copied but nobody cleans it up).

|contents| is base::Owned by the callback.  This means it will be deleted once
all references to the closure are deleted.  Documentation here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:143: contents =
new std::string;  // Owned by GetSourceFileCallback()
On 2013/09/09 09:15:56, Finnur wrote:
> Same problem here: GetSourceFileCallback passes this to the highlighter as a
> const ref but does nothing more with it.

See above :)

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
File extensions/browser/file_highlighter.cc (right):

https://codereview.chromium.org/23624002/diff/64001/extensions/browser/file_h...
extensions/browser/file_highlighter.cc:207: start_ = contents_.find('\n',
start_) + 1;
On 2013/09/09 09:15:56, Finnur wrote:
> The +1 makes the check for npos on line 213 invalid, no? Have you tried what
> happens if the file has no linebreaks?

Whoops!  It'll actually work, given valid input (the only valid line number for
a file with no linebreaks is 1, so we never enter the for-loop at 206, and we
get the entire file).  But fixed in https://codereview.chromium.org/23875013/. 
Thanks! :)

[Finnur, 2013-09-09 18:05:43]

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:108:
base::FilePath path = extension->path().Append(path_suffix);
> $ vim
> ~/.config/google-
chrome/<profile>/Extensions/<extension_id>/<version>/manifest.json
> will give you the same information, significantly easier...

The person controlling a compromised renderer does not have access to vim as
he/she is not the same person as the one sitting by the keyboard. So I'm not
sure what point you are trying to make here.

> It's possible that, if the caller is compromised, you could see any other
> file, yes.  But, is this really a concern? 

Compromised callers are a concern and we can't always anticipate how they will
use the small cracks they find. They've shown great ability to jump through
hoops in the past. As for being able to read arbitrary manifest.json files
(which is what you were referring to)... probably not a huge deal. Looks pretty
innocent, right?

Although there might be potential value in being able to detect which extensions
the user has installed (which you can do if you can request arbitrary
manifests). Also, people have on occasion distributed their private key with
their extension, so I wouldn't put it past them to store something sensitive in
the manifest. Yes, the extension author would look pretty dumb if it gets
exploited, but we'd also look a little dumb for enabling it. And then there's
the question whether any |other| random app out there has ever stored something
sensitive in a file called manifest.json?

Is it likely this would be exploited? No. This is very low on the list of what
would scare a security person. So low it feels a bit silly to be spending any
time debating the merits of this because a) it is super easy to fix and b) that
seems like the right thing to do.

Why not fix it? Do we lose anything?

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:132: contents =
new std::string;  // Owned by GetManifestFileCallback(    )
Thanks, I overlooked that.

[Devlin, 2013-09-09 18:26:14]

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/23624002/diff/64001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:108:
base::FilePath path = extension->path().Append(path_suffix);
On 2013/09/09 18:05:43, Finnur wrote:
> > $ vim
> > ~/.config/google-
> chrome/<profile>/Extensions/<extension_id>/<version>/manifest.json
> > will give you the same information, significantly easier...
> 
> The person controlling a compromised renderer does not have access to vim as
> he/she is not the same person as the one sitting by the keyboard. So I'm not
> sure what point you are trying to make here.
> 
> > It's possible that, if the caller is compromised, you could see any other
> > file, yes.  But, is this really a concern? 
> 
> Compromised callers are a concern and we can't always anticipate how they will
> use the small cracks they find. They've shown great ability to jump through
> hoops in the past. As for being able to read arbitrary manifest.json files
> (which is what you were referring to)... probably not a huge deal. Looks
pretty
> innocent, right?
> 
> Although there might be potential value in being able to detect which
extensions
> the user has installed (which you can do if you can request arbitrary
> manifests). Also, people have on occasion distributed their private key with
> their extension, so I wouldn't put it past them to store something sensitive
in
> the manifest. Yes, the extension author would look pretty dumb if it gets
> exploited, but we'd also look a little dumb for enabling it. And then there's
> the question whether any |other| random app out there has ever stored
something
> sensitive in a file called manifest.json?
> 
> Is it likely this would be exploited? No. This is very low on the list of what
> would scare a security person. So low it feels a bit silly to be spending any
> time debating the merits of this because a) it is super easy to fix and b)
that
> seems like the right thing to do.
> 
> Why not fix it? Do we lose anything?

Thanks for the explanation :)  Fixed in 23875013.