Hooking MHTML generation to the browser.

Hooking MHTML generation to the browser.

This CL adds a class that can be used to generate MHTML for the current
page of a tab.
It is not yet surfaced in the UI.

BUG=None
TEST=Run the browser tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89047

[Jay Civelli, 2011-06-09 22:15:12]

Randy,
Could you review the overall patch?

Michael,
Could you review the vfs_backend small refactoring? (I moved
GetFileHandleForProcess to base).

John,
Could you review the changes to content/browser?

[jam, 2011-06-09 22:32:03]

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:25: #include
"chrome/browser/download/mhtml_generation_manager.h"
you'll get a DEPS failure for this, since you can't include chrome code from
content (we're removing all existing includes, and are down to < 30.  adding
ones isn't allowed at this point).

the good news is that this is trivial to fix, just dispatch this message in
chrome/browser/renderer_host/chrome_render_message_filter.cc instead.  that's
where we filter chrome specific messages.

similarly, the IPC message, since it's chrome-specific, need to be in
chrome\common\render_messages.h instead of in content.

[Paweł Hajdan Jr., 2011-06-10 07:15:51]

Drive-by with testing comments.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_browsertest.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:22: FilePath
delete_on_teardown_;
Please use ScopedTempDir.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:33:
ScopedTestingBrowserProcess testing_browser_process_;
Why do you use a TestingBrowserProcess here? InProcessBrowserTests generally use
the real one.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:35:
DISALLOW_COPY_AND_ASSIGN(MHTMLGenerationTest);
nit: No need for that in the test fixture.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:63: // Make sure the
generated file has some contents.
Why not test that the contents match the expectations?

[Randy Smith (Not in Mondays), 2011-06-10 18:45:38]

Jay: Thanks for doing this; I'm excited to see it going in. 

A couple of caveats about "rdsmith: Review the entire CL": I don't feel
competent to review the VfsBackend/PlatformFile/Writing to file from renderer
section.  If Michael's doing that, you're probably ok with my not having done
so.    I'm also a little uncertain about judging the TODO below as noted.  

Having said that, this is pretty close--I only have a few comments.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_manager.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_manager.cc:71: LOG(ERROR) << "Failed to
create file";
I don't think this error's likely enough that you want to route it to a job
completed(failure) notification, not just log and drop it on the floor.

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
File chrome/renderer/mhtml_generator.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
chrome/renderer/mhtml_generator.cc:47: //                 message loop to
process other events.
Another area where I'm feeling my lack of competence to well-review the
code--I'm not sure how big a deal it is that this CL doesn't write the chunk in
deferred tasks.  My guess is that you're fine leaving this in a todo (if this
blocks for a while, it's just blocking the renderer process, and it'll be pretty
clear to the user why if they just did a save), but you might want to run this
particular issue by someone who's more familiar with the responsiveness
requirements of the renderer message loop--I've never worked in that space.

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
chrome/renderer/mhtml_generator.cc:55: if (base::WritePlatformFile(file_, i,
data + i, copy_size) == -1)
Shouldn't you handle the case where WritePlatformFile returns a number of writes
other than copy_size?  I'm not sure that can ever happen given our context, but
it's part of the interface.

[Randy Smith (Not in Mondays), 2011-06-10 19:49:58]

Quick followon comment.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_manager.h (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_manager.h:20:
BrowserThread::DeleteOnUIThread> {
This seems like it's a leak in waiting.  If the generation manager is destroyed
(or might be destroyed) during shutdown, this won't work, because shutdown
happens after first returning from the UI MessageLoop::Run() call, and so the
message sent to the UI thread when the last reference to the object goes will
result in a message being sent to a non-existent thread, and no destruction.

[jam, 2011-06-10 20:25:06]

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_manager.h (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_manager.h:20:
BrowserThread::DeleteOnUIThread> {
On 2011/06/10 19:49:58, rdsmith wrote:
> This seems like it's a leak in waiting.  If the generation manager is
destroyed
> (or might be destroyed) during shutdown, this won't work, because shutdown
> happens after first returning from the UI MessageLoop::Run() call, and so the
> message sent to the UI thread when the last reference to the object goes will
> result in a message being sent to a non-existent thread, and no destruction.

this is totally acceptable, and happens in lots of places in the code.  it's
assumed that with the usage of DeleteOnXThread that the delete might not happen.
 memory leaks on shutdown are fine.

[Jay Civelli, 2011-06-10 22:38:22]

Addressed all comments.
(sorry I sync'ed between patches, so diff to previous patch got polluted)

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_browsertest.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:22: FilePath
delete_on_teardown_;
On 2011/06/10 07:15:51, Paweł Hajdan Jr. wrote:
> Please use ScopedTempDir.

Done.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:33:
ScopedTestingBrowserProcess testing_browser_process_;
On 2011/06/10 07:15:51, Paweł Hajdan Jr. wrote:
> Why do you use a TestingBrowserProcess here? InProcessBrowserTests generally
use
> the real one.
OK, removed.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:35:
DISALLOW_COPY_AND_ASSIGN(MHTMLGenerationTest);
On 2011/06/10 07:15:51, Paweł Hajdan Jr. wrote:
> nit: No need for that in the test fixture.

OK

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_browsertest.cc:63: // Make sure the
generated file has some contents.
On 2011/06/10 07:15:51, Paweł Hajdan Jr. wrote:
> Why not test that the contents match the expectations?

The MHTML content contains date and random boundary string to separate sections.
That would require reusing the WebCore MHTML parser, or rewriting some parts of
it.

The goal of this test, is not really to check the validity of the MHTML
generated (this is the job of the WebKit tests) but more that we could really
generate the MHTML (especially that the sandbox did not prevent us from writing
to the file).
I added a comment to clarify that.

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
File chrome/browser/download/mhtml_generation_manager.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/browser/download/mhtm...
chrome/browser/download/mhtml_generation_manager.cc:71: LOG(ERROR) << "Failed to
create file";
On 2011/06/10 18:45:38, rdsmith wrote:
> I don't think this error's likely enough that you want to route it to a job
> completed(failure) notification, not just log and drop it on the floor.
Good point, now notifying of the failure,

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
File chrome/renderer/mhtml_generator.cc (right):

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
chrome/renderer/mhtml_generator.cc:47: //                 message loop to
process other events.
On 2011/06/10 18:45:38, rdsmith wrote:
> Another area where I'm feeling my lack of competence to well-review the
> code--I'm not sure how big a deal it is that this CL doesn't write the chunk
in
> deferred tasks.  My guess is that you're fine leaving this in a todo (if this
> blocks for a while, it's just blocking the renderer process, and it'll be
pretty
> clear to the user why if they just did a save), but you might want to run this
> particular issue by someone who's more familiar with the responsiveness
> requirements of the renderer message loop--I've never worked in that space.
I think it is OK too, but I'll try to find someone to agree on it.

http://codereview.chromium.org/7044095/diff/4001/chrome/renderer/mhtml_genera...
chrome/renderer/mhtml_generator.cc:55: if (base::WritePlatformFile(file_, i,
data + i, copy_size) == -1)
On 2011/06/10 18:45:38, rdsmith wrote:
> Shouldn't you handle the case where WritePlatformFile returns a number of
writes
> other than copy_size?  I'm not sure that can ever happen given our context,
but
> it's part of the interface.
Good point, now taking the returned number of bytes into account.

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:25: #include
"chrome/browser/download/mhtml_generation_manager.h"
On 2011/06/09 22:32:03, John Abd-El-Malek wrote:
> you'll get a DEPS failure for this, since you can't include chrome code from
> content (we're removing all existing includes, and are down to < 30.  adding
> ones isn't allowed at this point).
> 
> the good news is that this is trivial to fix, just dispatch this message in
> chrome/browser/renderer_host/chrome_render_message_filter.cc instead.  that's
> where we filter chrome specific messages.
> 
> similarly, the IPC message, since it's chrome-specific, need to be in
> chrome\common\render_messages.h instead of in content.

I knew I was doing something wrong!
Moved things around as you suggested.

[michaeln, 2011-06-10 23:34:46]

> Michael,
> Could you review the vfs_backend small refactoring? (I moved
> GetFileHandleForProcess to base).

refactoring lgtm

[jam, 2011-06-10 23:41:31]

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:25: #include
"chrome/browser/download/mhtml_generation_manager.h"
On 2011/06/10 22:38:22, Jay Civelli wrote:
> On 2011/06/09 22:32:03, John Abd-El-Malek wrote:
> > you'll get a DEPS failure for this, since you can't include chrome code from
> > content (we're removing all existing includes, and are down to < 30.  adding
> > ones isn't allowed at this point).
> > 
> > the good news is that this is trivial to fix, just dispatch this message in
> > chrome/browser/renderer_host/chrome_render_message_filter.cc instead. 
that's
> > where we filter chrome specific messages.
> > 
> > similarly, the IPC message, since it's chrome-specific, need to be in
> > chrome\common\render_messages.h instead of in content.
> 
> I knew I was doing something wrong!
> Moved things around as you suggested.

I don't see that?

[Jay Civelli, 2011-06-11 00:41:59]

On 2011/06/10 23:41:31, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
http://codereview.chromium.org/7044095/diff/4001/content/browser/renderer_hos...
> content/browser/renderer_host/render_message_filter.cc:25: #include
> "chrome/browser/download/mhtml_generation_manager.h"
> On 2011/06/10 22:38:22, Jay Civelli wrote:
> > On 2011/06/09 22:32:03, John Abd-El-Malek wrote:
> > > you'll get a DEPS failure for this, since you can't include chrome code
from
> > > content (we're removing all existing includes, and are down to < 30. 
adding
> > > ones isn't allowed at this point).
> > > 
> > > the good news is that this is trivial to fix, just dispatch this message
in
> > > chrome/browser/renderer_host/chrome_render_message_filter.cc instead. 
> that's
> > > where we filter chrome specific messages.
> > > 
> > > similarly, the IPC message, since it's chrome-specific, need to be in
> > > chrome\common\render_messages.h instead of in content.
> > 
> > I knew I was doing something wrong!
> > Moved things around as you suggested.
> 
> I don't see that?

Oops, I screwed up with Git. Should be good now.

[Paweł Hajdan Jr., 2011-06-11 18:04:45]

Code I commented in the drive-by LGTM with comments. Thanks!

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/download/mht...
File chrome/browser/download/mhtml_generation_browsertest.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/download/mht...
chrome/browser/download/mhtml_generation_browsertest.cc:23: FilePath
delete_on_teardown_;
This is now unused, please remove it.

http://codereview.chromium.org/7044095/diff/11008/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:241: if
(!mhtml_generation_manager_.get())
nit: Do we need this code in TestingBrowserProcess now, or can we just return
NULL and remove mhtml_generation_manager_ for simplicity?

[jam, 2011-06-11 19:10:32]

lgtm with the one change

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:510:
BrowserThread::UI, FROM_HERE, NewRunnableMethod(
if you want to dispatch this message on the UI thread, then just list the
message ID in OverrideThreadForMessage and set it to UI

[Randy Smith (Not in Mondays), 2011-06-13 17:46:58]

LGTM.

[commit-bot: I haz the power, 2011-06-13 22:00:27]

Can't process patch for file base/platform_file.h.
File's status is None, patchset upload is incomplete.

[Jay Civelli, 2011-06-13 22:45:41]

Brett,
Would you mind reviewing the small change to base/platform_file*

(just refactoring some code)

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/download/mht...
File chrome/browser/download/mhtml_generation_browsertest.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/download/mht...
chrome/browser/download/mhtml_generation_browsertest.cc:23: FilePath
delete_on_teardown_;
On 2011/06/11 18:04:45, Paweł Hajdan Jr. wrote:
> This is now unused, please remove it.

Done.

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:510:
BrowserThread::UI, FROM_HERE, NewRunnableMethod(
On 2011/06/11 19:10:33, John Abd-El-Malek wrote:
> if you want to dispatch this message on the UI thread, then just list the
> message ID in OverrideThreadForMessage and set it to UI

Done.

http://codereview.chromium.org/7044095/diff/11008/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/7044095/diff/11008/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:241: if
(!mhtml_generation_manager_.get())
On 2011/06/11 18:04:45, Paweł Hajdan Jr. wrote:
> nit: Do we need this code in TestingBrowserProcess now, or can we just return
> NULL and remove mhtml_generation_manager_ for simplicity?

Ah, good point, now returning NULL.

[brettw, 2011-06-13 23:12:36]

http://codereview.chromium.org/7044095/diff/2041/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7044095/diff/2041/base/platform_file.h#newcode147
base/platform_file.h:147: bool close_source_handle);
I'm a little worried that close_source_handle only does something on Windows. It
seems like any code that sets this to true then must have some ifdefed code to
close the handle somehow on Posix.

Currently, what happens is that almost all code either sends the duplicated
handle over IPC on Windows, or converts it to a FileDescriptor on Posix which
then encompases the auto close semantics.

I wonder if we should typedef HANDLE FileDescriptor on Windows and then have
GetFileHandleForProcess return a FileDescriptor. This would eliminate the chance
of forgetting to close the handle on Posix, and also allow us to delete the
other ifdef in the database_message_filter function you changed.

Actually, looking at this more, what I've described is exactly
PlatformFileForTransit in src/ipc/ipc_platform_file.h. How about moving your
function there and returning a PlatformFileForTransit?

If we do this we should probably use the implementation from
ShareHandleWithRemote in
http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/proxy/proxy_channel.cc?...

[piman, 2011-06-13 23:19:42]

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc#...
base/platform_file_posix.cc:197: return file;
from the peanut-gallery: this has a race condition.
Imagine something like:
int fd=open("myfile", O_RDONLY);
int remote_fd = GetFileHandleForProcess(fd, remote_process, false);
channel->Send(new MyMessage(FileDescriptor(remote_fd, false)));
close(fd);

Then, since remote_fd==fd, the close(fd) may close remote_fd before the IPC
message gets sent, in which case the receiving process won't receive a valid fd
(or at least not the one it should)

A good way to ensure no race is to always returns a fd that the caller owns:
close_source_handle ? file : dup(file)

And then the caller is responsible for sending a FileDescriptor(remote_fd,
true), avoiding the race in all cases.

[Jay Civelli, 2011-06-14 00:19:27]

http://codereview.chromium.org/7044095/diff/2041/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7044095/diff/2041/base/platform_file.h#newcode147
base/platform_file.h:147: bool close_source_handle);
On 2011/06/13 23:12:36, brettw wrote:
> I'm a little worried that close_source_handle only does something on Windows.
It
> seems like any code that sets this to true then must have some ifdefed code to
> close the handle somehow on Posix.
> 
> Currently, what happens is that almost all code either sends the duplicated
> handle over IPC on Windows, or converts it to a FileDescriptor on Posix which
> then encompases the auto close semantics.
> 
> I wonder if we should typedef HANDLE FileDescriptor on Windows and then have
> GetFileHandleForProcess return a FileDescriptor. This would eliminate the
chance
> of forgetting to close the handle on Posix, and also allow us to delete the
> other ifdef in the database_message_filter function you changed.
> 
> Actually, looking at this more, what I've described is exactly
> PlatformFileForTransit in src/ipc/ipc_platform_file.h. How about moving your
> function there and returning a PlatformFileForTransit?
> 
> If we do this we should probably use the implementation from
> ShareHandleWithRemote in
>
http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/proxy/proxy_channel.cc?...

OK, moved that method to ipc_plaform_file,cc and reusing the code from ppapi
procy_channel.cc.

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc#...
base/platform_file_posix.cc:197: return file;
On 2011/06/13 23:19:42, piman wrote:
> from the peanut-gallery: this has a race condition.
> Imagine something like:
> int fd=open("myfile", O_RDONLY);
> int remote_fd = GetFileHandleForProcess(fd, remote_process, false);
> channel->Send(new MyMessage(FileDescriptor(remote_fd, false)));
> close(fd);
> 
> Then, since remote_fd==fd, the close(fd) may close remote_fd before the IPC
> message gets sent, in which case the receiving process won't receive a valid
fd
> (or at least not the one it should)
> 
> A good way to ensure no race is to always returns a fd that the caller owns:
> close_source_handle ? file : dup(file)
> 
> And then the caller is responsible for sending a FileDescriptor(remote_fd,
> true), avoiding the race in all cases.
Good point!
I moved that code to ipc_platform_file.cc, and this is what it now does.

[piman, 2011-06-14 00:31:22]

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7044095/diff/2041/base/platform_file_posix.cc#...
base/platform_file_posix.cc:197: return file;
On 2011/06/14 00:19:27, Jay Civelli wrote:
> On 2011/06/13 23:19:42, piman wrote:
> > from the peanut-gallery: this has a race condition.
> > Imagine something like:
> > int fd=open("myfile", O_RDONLY);
> > int remote_fd = GetFileHandleForProcess(fd, remote_process, false);
> > channel->Send(new MyMessage(FileDescriptor(remote_fd, false)));
> > close(fd);
> > 
> > Then, since remote_fd==fd, the close(fd) may close remote_fd before the IPC
> > message gets sent, in which case the receiving process won't receive a valid
> fd
> > (or at least not the one it should)
> > 
> > A good way to ensure no race is to always returns a fd that the caller owns:
> > close_source_handle ? file : dup(file)
> > 
> > And then the caller is responsible for sending a FileDescriptor(remote_fd,
> > true), avoiding the race in all cases.
> Good point!
> I moved that code to ipc_platform_file.cc, and this is what it now does.

Great, LGTM on that.

[brettw, 2011-06-14 15:17:24]

LGTM

http://codereview.chromium.org/7044095/diff/17002/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7044095/diff/17002/base/platform_file.h#newcode19
base/platform_file.h:19: #include "base/process.h"
Is this necessary any more?

http://codereview.chromium.org/7044095/diff/17002/ipc/ipc_platform_file.cc
File ipc/ipc_platform_file.cc (right):

http://codereview.chromium.org/7044095/diff/17002/ipc/ipc_platform_file.cc#ne...
ipc/ipc_platform_file.cc:28: // dup()ing and close()ing.
Can you add to this comment, I had to ask Antoine why we needed the dup(): "When
we're not closing the source, we need to duplicate the handle and take ownership
of that. The reason is that this function is often used to generate IPC
messages, and the handle must remain valid until it's sent to the other process
from the I/O thread. Without the dup, calling code might close the source handle
before the message is sent, creating a race condition."

[commit-bot: I haz the power, 2011-06-14 19:35:13]

Change committed as 89047