Implement ManifestManager to handle manifest in content/

Implement ManifestManager to handle manifest in content/.

This can be used from the renderer process or the browser
process. Requesting the manifest can be done via one call, a
callback has to be passed and will run with the manifest
passed as parameter. A failure will return the empty
manifest.

Some more logic could be added like caching the manifest in
the browser process or keeping track of the manifest dirty
state in the browser process but those things can be added
transparently later.

BUG=366145
Committed: https://crrev.com/efdca9d8f31e7aa0ce7cb7a2eff5f08dead2498b
Cr-Commit-Position: refs/heads/master@{#295085}

[mlamouri (slow - plz ping), 2014-09-09 12:40:38]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, kenneth.r.christiansen@intel.com,
mvanouwerkerk@chromium.org, palmer@chromium.org

[mlamouri (slow - plz ping), 2014-09-09 12:40:39]

Kenneth and Michael, could you take a look at this CL as non-owners?

Chris, could you review the IPC messages changes?
Jochen, could you review everything else?

Thanks! :)

[Michael van Ouwerkerk, 2014-09-09 15:34:56]

https://codereview.chromium.org/537053002/diff/80001/content/renderer/manifes...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/537053002/diff/80001/content/renderer/manifes...
content/renderer/manifest/manifest_manager.cc:39:
request_ids_.push_back(request_id);
Why store this list of ids separately? You can bind it into the callback.

[jochen (gone - plz use gerrit), 2014-09-10 08:02:44]

https://codereview.chromium.org/537053002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/537053002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:333: virtual void
GetManifest(const GetManifestCallback&) OVERRIDE;
why does this have to be on WebContents?

https://codereview.chromium.org/537053002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/537053002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3200:
manifest_manager_->ManifestChanged();
can you add a render frame observer callback instead

[mlamouri (slow - plz ping), 2014-09-10 17:06:20]

Comments applied.

I've also applied some changes we talked to with Michael with regards to the
RenderFrame dying while a Manifest is being requested. It makes the host manager
a bit more complex, unfortunately.

PTAL.

https://codereview.chromium.org/537053002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/537053002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:333: virtual void
GetManifest(const GetManifestCallback&) OVERRIDE;
On 2014/09/10 08:02:43, jochen wrote:
> why does this have to be on WebContents?

|manifest_manager_hosts_| has to live in WebContentsImpl AFAICT and the
candidates consumer of this API (::GetManifest()) are all using WebContents so
it seemed natural. In addition, my understanding is that WebContents is some
kind of tab representation and we really expect consumers to try to get the
manifest of the top-level frame, trying to access the manifest of another frame
is not something we currently expect.

What would you recommend?

https://codereview.chromium.org/537053002/diff/80001/content/renderer/manifes...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/537053002/diff/80001/content/renderer/manifes...
content/renderer/manifest/manifest_manager.cc:39:
request_ids_.push_back(request_id);
On 2014/09/09 15:34:56, Michael van Ouwerkerk wrote:
> Why store this list of ids separately? You can bind it into the callback.

Done.

https://codereview.chromium.org/537053002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/537053002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3200:
manifest_manager_->ManifestChanged();
On 2014/09/10 08:02:43, jochen wrote:
> can you add a render frame observer callback instead

Done.

[kenneth.christiansen, 2014-09-11 09:33:07]

kenneth.christiansen@gmail.com changed reviewers:
+ kenneth.christiansen@gmail.com

[kenneth.christiansen, 2014-09-11 09:33:10]

Some nits:

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:96: // manifest should return a
properly filed manifest.
filled*

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:105:
EXPECT_FALSE(manifest().IsEmpty());
So this test is supposed to fail for now? empty doesn't sounds like "properly
filled" manifest

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:108: // If a page changes
manifest during its runtime, requesting the manifest should
s/runtime/life-time/ ?

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:25: return it->second;
Wouldn't it be nicer in one line?

return (it != pending_callbacks_.end()) ? it->second : 0;

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:38:
it.GetCurrentValue()->Run(Manifest());
Is that a failure state?

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:77: LOG(ERROR) << "Received a
ManifestResponse for a RFH not waiting for it";
without waiting for it?

Unexpectedly received a ManifestResponse for a RFH without waiting for it

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:84: "with no associated
callabck.";
spelling issue: callabck.

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.h (right):

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:55: void DownloadManifest();
FetchManifest?

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:56: void
OnManifestFetchComplete(const blink::WebURLResponse& response,
Here Fetch is used, and not Download as above

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:63: // may have a manifest, if
false, it can't have one.
Maybe a bit more explanation to why it cannot have one.

[mlamouri (slow - plz ping), 2014-09-11 10:30:32]

PTAL

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:96: // manifest should return a
properly filed manifest.
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> filled*

Done.

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:105:
EXPECT_FALSE(manifest().IsEmpty());
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> So this test is supposed to fail for now? empty doesn't sounds like "properly
> filled" manifest

dummy-manifest.json isn't empty. The test checks that the manifest is not empty.

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:108: // If a page changes
manifest during its runtime, requesting the manifest should
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> s/runtime/life-time/ ?

Done.

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:25: return it->second;
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> Wouldn't it be nicer in one line?
> 
> return (it != pending_callbacks_.end()) ? it->second : 0;

Done.

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:38:
it.GetCurrentValue()->Run(Manifest());
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> Is that a failure state?

Yes. An empty manifest is returned if no manifest was found, the manifest failed
to parse, etc.

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:77: LOG(ERROR) << "Received a
ManifestResponse for a RFH not waiting for it";
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> without waiting for it?
> 
> Unexpectedly received a ManifestResponse for a RFH without waiting for it

"Unexpected ManifestResponse to RFH."

https://codereview.chromium.org/537053002/diff/100001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:84: "with no associated
callabck.";
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> spelling issue: callabck.

Done.

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.h (right):

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:55: void DownloadManifest();
On 2014/09/11 09:33:10, kenneth.christiansen wrote:
> FetchManifest?

Done.

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:56: void
OnManifestFetchComplete(const blink::WebURLResponse& response,
On 2014/09/11 09:33:09, kenneth.christiansen wrote:
> Here Fetch is used, and not Download as above

Done.

https://codereview.chromium.org/537053002/diff/100001/content/renderer/manife...
content/renderer/manifest/manifest_manager.h:63: // may have a manifest, if
false, it can't have one.
On 2014/09/11 09:33:10, kenneth.christiansen wrote:
> Maybe a bit more explanation to why it cannot have one.

Done.

[Michael van Ouwerkerk, 2014-09-11 11:03:14]

lgtm with nits

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:75: LOG(ERROR) << "Unexpected
ManifestResponse to RFH.";
Nit: avoid abbreviations, spell out RenderFrameHost

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.h (right):

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.h:56: // Returns the CallbackMap
associated to the given RenderFrameHost, or null.
"associated with"

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:47: base::Unretained(this),
request_id));
Are you sure now that Unretained is the right thing to use?

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:95: base::Unretained(this)));
Also here, you checked that Unretained is the right thing to use?

[mlamouri (slow - plz ping), 2014-09-11 11:11:49]

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:75: LOG(ERROR) << "Unexpected
ManifestResponse to RFH.";
On 2014/09/11 11:03:14, Michael van Ouwerkerk wrote:
> Nit: avoid abbreviations, spell out RenderFrameHost

Done.

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.h (right):

https://codereview.chromium.org/537053002/diff/120001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.h:56: // Returns the CallbackMap
associated to the given RenderFrameHost, or null.
On 2014/09/11 11:03:14, Michael van Ouwerkerk wrote:
> "associated with"

Done.

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:47: base::Unretained(this),
request_id));
On 2014/09/11 11:03:14, Michael van Ouwerkerk wrote:
> Are you sure now that Unretained is the right thing to use?

Yes.

https://codereview.chromium.org/537053002/diff/120001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:95: base::Unretained(this)));
On 2014/09/11 11:03:14, Michael van Ouwerkerk wrote:
> Also here, you checked that Unretained is the right thing to use?

Yes, the reason being that |fetcher_| is owned by |this| and |this| will
Cancel() the fetch in the dtor so worse case, the callback will be called at
that point. It shouldn't happen that |fetcher_| outlive |this| and runs the
callback after |this| is destroyed.

[palmer, 2014-09-11 20:47:31]

Where is manifest.h, and struct Manifest? I don't have it in my source tree, and
it isn't in this CL.

It looks like the struct contains 2 strings, and nothing else? Make sure there's
some sane limit on the size and lexical/syntactic content of those strings.

[mlamouri (slow - plz ping), 2014-09-12 12:08:33]

On 2014/09/11 20:47:31, Chromium Palmer wrote:
> It looks like the struct contains 2 strings, and nothing else? Make sure
there's
> some sane limit on the size and lexical/syntactic content of those strings.

For the moment, the struct contains only two strings. It is meant to evolve.

Would that be fine if I arbitrary truncate the strings before sending them to
the browser process? Title seems to be using 4*1024 [1]. I could use the same
value.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[palmer, 2014-09-12 18:34:14]

On 2014/09/12 12:08:33, Mounir Lamouri wrote:
> On 2014/09/11 20:47:31, Chromium Palmer wrote:
> > It looks like the struct contains 2 strings, and nothing else? Make sure
> there's
> > some sane limit on the size and lexical/syntactic content of those strings.
> 
> For the moment, the struct contains only two strings. It is meant to evolve.
> 
> Would that be fine if I arbitrary truncate the strings before sending them to
> the browser process? Title seems to be using 4*1024 [1]. I could use the same
> value.

4 KiB is fine (higher would be fine too, but 4 KiB is great).

But, there are some issues:

* For security purposes, enforce the limit in the browser process, not in the
renderer/other non-browser process.
* for efficiency purposes, enforce the limit in the renderer/other non-browser
process (i.e. to avoid wasting time and space sending a message the browser will
reject anyway).
* Truncation may be, or tickle, a bug. If you find you have to truncate a
string, that is probably an error that should be handled, rather than truncating
and hoping it's OK.

Also, are there lexical or syntactic requirements to these strings, or are they
really just arbitrary sequences of characters? If the latter, I usually use
vector<uint8_t> to express that fact.

See
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
for background.

[mlamouri (slow - plz ping), 2014-09-15 13:44:24]

I've uploaded a new patch with IPC security comments taken into consideration.
PTAL.

On 2014/09/12 18:34:14, Chromium Palmer wrote:
> 4 KiB is fine (higher would be fine too, but 4 KiB is great).
> 
> But, there are some issues:
> 
> * For security purposes, enforce the limit in the browser process, not in the
> renderer/other non-browser process.

Done.

> * for efficiency purposes, enforce the limit in the renderer/other non-browser
> process (i.e. to avoid wasting time and space sending a message the browser
will
> reject anyway).

Done.

> * Truncation may be, or tickle, a bug. If you find you have to truncate a
> string, that is probably an error that should be handled, rather than
truncating
> and hoping it's OK.

The string is coming from an arbitrary file. I guess in that case it is not a
bug.

> Also, are there lexical or syntactic requirements to these strings, or are
they
> really just arbitrary sequences of characters? If the latter, I usually use
> vector<uint8_t> to express that fact.

That sounds reasonable for a buffer of arbitrary bytes but what we have here is
meant to be a human readable string. It would be odd to have it represented as a
vector of characters.

[palmer, 2014-09-15 21:26:17]

> > * Truncation may be, or tickle, a bug. If you find you have to truncate a
> > string, that is probably an error that should be handled, rather than
> truncating
> > and hoping it's OK.
> 
> The string is coming from an arbitrary file. I guess in that case it is not a
> bug.

OK.

> > Also, are there lexical or syntactic requirements to these strings, or are
> they
> > really just arbitrary sequences of characters? If the latter, I usually use
> > vector<uint8_t> to express that fact.
> 
> That sounds reasonable for a buffer of arbitrary bytes but what we have here
is
> meant to be a human readable string. It would be odd to have it represented as
a
> vector of characters.

OK.

[palmer, 2014-09-15 21:29:21]

LGTM

[mlamouri (slow - plz ping), 2014-09-16 09:14:07]

Jochen, PTAL.

[jochen (gone - plz use gerrit), 2014-09-16 14:41:39]

lgtm with nits

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:41: };
disallow copy and assign

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:75: LOG(ERROR) << "Unexpected
ManifestResponse to RenderFrameHost.";
DVLOG(1) should be enough

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:81: LOG(ERROR) << "Received a
request_id (" << request_id << ") from renderer "
DVLOG(1). Also kill the renderer because an invalid IPC message?

[mlamouri (slow - plz ping), 2014-09-16 15:32:37]

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_browsertest.cc:41: };
On 2014/09/16 14:41:39, jochen wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:75: LOG(ERROR) << "Unexpected
ManifestResponse to RenderFrameHost.";
On 2014/09/16 14:41:39, jochen wrote:
> DVLOG(1) should be enough

Done.

https://codereview.chromium.org/537053002/diff/160001/content/browser/manifes...
content/browser/manifest/manifest_manager_host.cc:81: LOG(ERROR) << "Received a
request_id (" << request_id << ") from renderer "
On 2014/09/16 14:41:39, jochen wrote:
> DVLOG(1). Also kill the renderer because an invalid IPC message?

Done. I've added some code to kill the renderer in that case. Also in the
previous case.

[mlamouri (slow - plz ping), 2014-09-16 15:33:18]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-16 15:34:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/537053002/180001

[commit-bot: I haz the power, 2014-09-16 15:51:30]

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

[commit-bot: I haz the power, 2014-09-16 15:51:32]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56550)

[mlamouri (slow - plz ping), 2014-09-16 15:57:23]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-16 15:58:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/537053002/200001

[commit-bot: I haz the power, 2014-09-16 16:56:29]

Committed patchset #11 (id:200001) as e599179a70c53af300874c5e2dbea8c1bbb0a2c4

[commit-bot: I haz the power, 2014-09-16 16:57:59]

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