ABANDONED: OOPIFs: Moving stitching of local paths from renderer to browser process.

ABANDONED CL - REPLACED WITH crrev.com/1413103003

OOPIFs: Moving stitching of local paths from renderer to browser process.

Context
=======

Save-Page-As in complete-html mode replaces links to subframes and
savable resources (i.e. img elements) with paths to locally saved files.
This replacement used to be done in the renderer process, but has to be
moved to the browser process, because with OOP iframes renderer process
is not able to look into any data belonging to subframes living in
another process (and "any data" covers URLs of subframes or lists of
resources contained in subframes).

Before this CL, saving (as complete html) a page with cross-site iframes in
OOPIFs mode would crash the renderer process.  After this CL there is no crash
(and the test in save_page_browsertest.cc should prevent regressions).


IPC Messages
============

This CL replaces the following 2 IPC messages:

    IPC_MESSAGE_ROUTED3(FrameMsg_GetSerializedHtmlWithLocalLinks,
                        std::vector<GURL> /* urls that have local copy */,
                        std::vector<base::FilePath> /* paths of local copy */,
                        base::FilePath /* local directory path */)

    IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializedHtmlWithLocalLinksResponse,
                        GURL /* frame URL */,
                        std::string /* data buffer */,
                        int32 /* complete status */)

with the following 5 IPC messages:

    IPC_MESSAGE_ROUTED0(FrameMsg_GetSerializedHtmlWithLocalLinks);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedHtmlFragment,
                        std::string /* data buffer */);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForChildFrame,
                        int /* RenderFrame's or RenderFrameProxy's routing id
                               for the child frame we need a local path for.*/);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForSavableResource,
                        GURL /* savable resource link */);

    IPC_MESSAGE_ROUTED0(FrameHostMsg_SerializedEndOfFrame)

Notice that
- Urls and local paths are no longer passed from browser to renderer
  processes.
- Renderer responses have been split into more granular responses (html
  fragment, local path for subresource, local path for child frame, end
  of data).

All the other changes in the CL are a consequence of the IPC change outlined above.


Other notes
===========

Mapping frames and subframes to SaveItems is done via frame_tree_node_id.

Note that the old WebPageSerializerImpl code [1] did *not* escape file paths
embedded in serializer output as html attribute values.  The current CL
preserves this behavior (in SavePackage::SerializeLocalPathForSaveItem).

[1] https://chromium.googlesource.com/chromium/blink/+/9d8944796d0b1208175177cda807910385f09d0b/Source/web/WebPageSerializerImpl.cpp#332


BUG=526786

[Łukasz Anforowicz, 2015-09-25 18:10:01]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2015-09-25 18:10:01]

Daniel, could you please take a look?

[Charlie Reis, 2015-09-25 19:52:09]

Quick side note that I forgot to mention earlier: Please CC
site-isolation-reviews@chromium.org on OOPIF related CLs, which helps the team
keep up with related activity.  Thanks!

[dcheng, 2015-09-30 05:37:33]

The Blink bits looked fine.

I reviewed the //content side, and individually, the pieces look fairly
reasonable. However, I think this .cc could benefit from an implementation
comment that describes how all the puzzle pieces fit together. In particular:
- the lifetime of SaveItems: how are they created, when are they destroyed, who
owns them.
- how SaveItems transition between waiting_item_queue, url_to_save_item_,
in_progress_items_, and saved_{success,failed}_items_ (this seems to be
essentially a state machine of SaveItems).

https://codereview.chromium.org/1373573002/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1104: FrameTreeNode* frame_tree_node =
It seems like this code would benefit from a FTN -> SaveItem map.

Also, I think that frames should probably be indexed by FTN ID rather than the
frame URL, but that can change in a separate patch.

https://codereview.chromium.org/1373573002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1166: for (const auto pair :
in_progress_items_) {
const auto&, since this copies otherwise.

https://codereview.chromium.org/1373573002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1264: SaveItem*& save_item =
url_to_save_item_[u];
I think this is a bit too clever =)

https://codereview.chromium.org/1373573002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/1373573002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializerClient.h:54: // inclusion as a
value of a double-quoted html attribute.
So essentially, this is only used for subframe resources.

https://codereview.chromium.org/1373573002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializerClient.h:61: // escaped for
inclusion as a value of a double-quoted html attribute.
And this is used for all other types of savable resources, right?

[Łukasz Anforowicz, 2015-10-01 20:39:58]

Thanks Daniel.  Could you take another look please?

Please note that in addition to addressing your feedback, I also fixed some
rather embarrassing issues that I've found via loosely related test changes I've
landed earlier in crrev.com/1362973003:

WebPageSerializerImpl.cpp changes:

- We need to flush the accumulated html buffer before calling
writeLocalPathForFoo.  The previous snapshot did add a flush, but the flush was
happening too late.

SavePackage.cc changes:

- After CompleteSavableResourceLinksResponse, SaveItems in waiting_item_queue_
need to be ordered so that savable resources are first and subframes are last. 
Various pieces in the old code depend on this ordering - examples:
DoSavingProcess (checks save_source() of SaveItem at the front()),
GetSerializedHtmlWithLocalLinks (checks that all savable resources have a local
path).  I couldn't decide for a while what the best fix would be (considered
splitting waiting_item_queue_ into 2 queues - one for savable resources and one
for subframes), but in the end thought that simply sorting waiting_item_queue_
will be most explicit statement about the ordering and also most straightfoward
change that doesn't requires touching too many pieces of code.

- Old code was building local, relative paths (i.e. "./page_files/img.png") by
string manipulation.  New code uses base::FilePath - this means 2 things: 1) no
"./" (this seems fine) and 2) default normalization to backslashes on Windows
(probably not ok - the saved html should be portable).  I fixed #2 by forcing
normalization to forward slash.

Also - note that WebPageSerializerTest.cpp is new in patchset#6 - after rebasing
I needed to make edits here related to WebPageSerializerClient interface.

On 2015/09/30 05:37:33, dcheng wrote:
> The Blink bits looked fine.
> 
> I reviewed the //content side, and individually, the pieces look fairly
> reasonable. However, I think this .cc could benefit from an implementation
> comment that describes how all the puzzle pieces fit together. In particular:
> - the lifetime of SaveItems: how are they created, when are they destroyed,
who
> owns them.

This is to some extent already covered by the class-level comment for
SavePackage.

> - how SaveItems transition between waiting_item_queue, url_to_save_item_,
> in_progress_items_, and saved_{success,failed}_items_ (this seems to be
> essentially a state machine of SaveItems).

I've extended the class-level SavePackage comment with description of how
SaveItems move between the 4 container fields.

https://chromiumcodereview.appspot.com/1373573002/diff/20001/content/browser/...
File content/browser/download/save_package.cc (right):

https://chromiumcodereview.appspot.com/1373573002/diff/20001/content/browser/...
content/browser/download/save_package.cc:1104: FrameTreeNode* frame_tree_node =
On 2015/09/30 05:37:32, dcheng wrote:
> It seems like this code would benefit from a FTN -> SaveItem map.

Good point.  Done.

I also found out that looking inside in_progress_items_ was wrong (because a
subresource or subframe might have already finished saving and be moved from
in_progress_items_ into saved_success_items_).  Having a map that points to
SaveItem* helps with this as well.

Introducing the map also means that I don't really need to modify SaveItem to
store frame_tree_node_id_ as a field.  I am keeping other SaveItem modifications
(to set referrer separately from the construction);  OTOH, preferring
referrer-from-savable-resources might not be needed once we don't dedupe frames
based on their URL (but then you are right that this should probably be in a
separate CL).

> Also, I think that frames should probably be indexed by FTN ID rather than the
> frame URL, but that can change in a separate patch.

Right.  I've added a TODO and opened crbug.com/538188 to track this.

https://chromiumcodereview.appspot.com/1373573002/diff/20001/content/browser/...
content/browser/download/save_package.cc:1264: SaveItem*& save_item =
url_to_save_item_[u];
On 2015/09/30 05:37:32, dcheng wrote:
> I think this is a bit too clever =)

Done.  (fixed by assigning to a new invocation of map[], rather than assigning
to the stored reference)

https://chromiumcodereview.appspot.com/1373573002/diff/40001/third_party/WebK...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://chromiumcodereview.appspot.com/1373573002/diff/40001/third_party/WebK...
third_party/WebKit/public/web/WebPageSerializerClient.h:54: // inclusion as a
value of a double-quoted html attribute.
On 2015/09/30 05:37:32, dcheng wrote:
> So essentially, this is only used for subframe resources.

Right.  writeLocalPathFor*SubFrame*

https://chromiumcodereview.appspot.com/1373573002/diff/40001/third_party/WebK...
third_party/WebKit/public/web/WebPageSerializerClient.h:61: // escaped for
inclusion as a value of a double-quoted html attribute.
On 2015/09/30 05:37:32, dcheng wrote:
> And this is used for all other types of savable resources, right?

Right.  writeLocalPathFor*SavableResource*

[dcheng, 2015-10-05 07:53:08]

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (left):

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:1221: if (frame_url.is_valid())
It seems like we don't have this check anymore?

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:137: auto rfhi =
static_cast<RenderFrameHostImpl*>(render_frame_host);
Nit: my personal preference is to annotate autos holding pointers with auto* to
make it 100% clear.

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:1036:
DCHECK_EQ(it.second->save_source(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
Nit: there's a weak convention of DCHECK_EQ(expected, actual) to match how the
gtest macros work.

https://codereview.chromium.org/1373573002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/1373573002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializerClient.h:59: //
|savebleResourceURL|.  WebPageSerializerClient is responsible for mapping
s/saveble/savable/

[Łukasz Anforowicz, 2015-10-05 17:57:56]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2015-10-05 17:57:57]

Charlie, could you please take a look?

Daniel suggested that this CL might be ready to get a review from a //content
owner - in particular he points out that there are some potentially
controversial parts like the const_casts to achieve const-correctness, and it'd
be good to see how the //content OWNERS feel about things like that (the
const-related changes mostly follow the recipe from Meyers and look ok to me,
but the const_cast<const FrameMap&> cast might be more controversial [although
it seems to me that this still preserves externally observable constness]).

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (left):

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:1221: if (frame_url.is_valid())
On 2015/10/05 07:53:08, dcheng wrote:
> It seems like we don't have this check anymore?

Thanks for catching this.  I am not sure how this has happened :-(

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:137: auto rfhi =
static_cast<RenderFrameHostImpl*>(render_frame_host);
On 2015/10/05 07:53:08, dcheng wrote:
> Nit: my personal preference is to annotate autos holding pointers with auto*
to
> make it 100% clear.

Done.

https://codereview.chromium.org/1373573002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:1036:
DCHECK_EQ(it.second->save_source(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
On 2015/10/05 07:53:08, dcheng wrote:
> Nit: there's a weak convention of DCHECK_EQ(expected, actual) to match how the
> gtest macros work.

Thanks for catching this.  I knew about this convention and somehow managed to
forget to apply this here...

https://codereview.chromium.org/1373573002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/1373573002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializerClient.h:59: //
|savebleResourceURL|.  WebPageSerializerClient is responsible for mapping
On 2015/10/05 07:53:08, dcheng wrote:
> s/saveble/savable/

Done.

[Łukasz Anforowicz, 2015-10-06 19:42:43]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org
- creis@chromium.org

[Łukasz Anforowicz, 2015-10-06 19:42:43]

Nick, could you please take a look?

[ncarter (slow), 2015-10-08 20:57:56]

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:166:
contents_id_(web_contents->GetRenderProcessHost()->GetID()),
Not introduced by your cl, but just wanted to put it on your radar: This is
yucky, since two WC's can easily share a RPH. If you've got cycles to fix it (in
a separate patch), I might suggest using the FTN id of the root FrameTreeNode of
the WebContents. It ought to be globally unique and stable over the lifetime of
a tab/WebContents.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1004: bool handled = true;
Consider adding here:

RenderFrameHostImpl* sender =
static_cast<RenderFrameHostImpl*>(render_frame_host);

And using that as the argument to WITH_PARAM. This would allow your handler
methods to accept an RFHImpl (if needed), and might let you get rid of helpers
like GetFrameTreeNodeId.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1048: DCHECK_LT(0,
number_of_frames_pending_response_);
Another "fyi, no action needed, just want to put this on your radar" comment: 

This DCHECK (which this CL doesn't add) is valid because we don't skip over
crashed subframes or mainframes. You can see this in practice: navigate to
"about:crash" to get a sad tab, and hit Ctrl+S. This creates a save page
operation that'll never terminate.

That's probably not a very serious bug (I think the URL-based deduping issues
you've uncovered are way more serious, for example). But it does seem somewhat
plausible to me, that someone would want to save-page something that has a
crashed subframe.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1064: SaveItem* save_item =
frame_tree_node_id_to_save_item_[frame_tree_node_id];
What if it's not in the map?

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1093: ->GetFrameTree()
If you passed in an RFHImpl here, you could alternatively get this via
sender->frame_tree_node()->frame_tree(). There's actually a subtle difference
between the two: when an interstitial is showing, it creates a second frame tree
for its content, so in code that needs to work with interstitial RFH's, going to
the WebContents would be a mistake. For save page, the difference doesn't
matter, since save-page seems to be disabled while an interstitial is showing.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1097: return;
Would it be worthwhile here to validate that frame_tree_node->parent() ==
sender->frame_tree_node()?

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1100:
frame_tree_node_id_to_save_item_[frame_tree_node->frame_tree_node_id()];
What if it's not in the map?

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1138:
number_of_frames_pending_response_--;
Shouldn't save_item()->state() be transitioned to something else here (as a
defense against OnSerializedEndOfFrame being sent multiple times for a frame)?

When I'm looking at browser-process code like this, I'm always asking the
question: could an attacker-controlled renderer process get me to do something I
didn't intend here? One broad defense for that is to avoid processing a response
message if we can't match it up to a request.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1171: // TODO(lukasza): Escape
attr_value (&quot; + &amp;).
My understanding is that there are no security considerations here, really,
since the renderer can by design write arbitrary content to the file.

But escaping could matter for correctness, if URL-important characters (like #
or ?) or context-important characters (like ") were to find their way into
saved_main_directory_path_ or save_item->file_name(). So, what kind of escaping
needs to happen here, exactly? Should we pass it through GURL for
canonicalization?

I'm OK with leaving this as a TODO, provided a CL to address it is right around
the corner. Doubly okay if that CL includes unittests that exercise the weird
cases (filename-reserved chars, url chars, PRN, COM9, etc). Over-escaping is
just as likely to be a cause of trouble as under-escaping, and it seems like
there's already some canonicalization code in place.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1233: if (save_item == nullptr) {
You seem to be preferring operator[] throughout instead of map::find(), which
means that a failed lookup can wind up with a nullptr getting into the map.  Why
is that? With the auto keyword, you can do it pretty concisely, and for
readability, you can store it->second into a SaveItem* variable once you've
verified it != map.end().

Here, even though the null value is replaced a couple lines later, I did a
double-take before realizing that's what was going on -- it's not a pattern I'm
used to seeing.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1269: std::sort(
I wonder if we ought to use stable_sort here to avoid a possible source of
nondeterminism / cross-platform differences? (this assumes, of course, that
waiting_item_queue_ is already in some kind of relative tree-order, which may
not be the case at all? If the order is random going in, sort() won't make it
worse)

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1275: (y->save_source() ==
SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
Would this be easier to comprehend if we wrote it as:

x != y && y == SAVE_FILE_FROM_DOM ?

(I'm honestly not sure. I was confused at first by your version, but once I
figured it out, it looks obvious. Leaving it up to you.)

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.h:196: // "blanks" that we can replace
with links to local paths).
Make this parenthetical clause its own sentence, something like: Links will be
represented as "blanks", which we'll replace with local paths later.

Also: is "blanks" the right term to use? I don't see it used anywhere else in
save_package.cc.

https://codereview.chromium.org/1373573002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:1182:
IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForSubFrame,
I think ChildFrame is a better term than SubFrame for this context, to be
consistent with FrameHostMsg_CreateChildFrame and FrameMsg_SetChildFrameSurface.

If you do rename this, don't forget to update the instance in the (awesome) CL
description too.

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:60: static const RenderFrame*
FromWebFrame(const blink::WebFrame* web_frame);
I appreciate the justification you gave for adding these const overrides in the
CL description, but for reasons I'll explain below, we shouldn't need them. The
TL;DR is that "const RenderFrame*" ought to be useless.

The rules we try to enforce for the content api (which this, being
content/public, is part of) are posted here:
http://www.chromium.org/developers/content-module/content-api. One of them says:
"Don't add the const identifier to interfaces. ... For interfaces implemented by
content, the implementation details doesn't have to be exposed."  Per that
policy, all the methods of RenderFrame ought to be non-const, and it's common
for RenderFrame's const-hostility to spilling over to code that has to interacts
with it. Although there are a couple const methods today, they're in violation
of the policy and we'd like to eradicate them too.

Yes, this goes against the Effective C++ recommendation of "use const wherever
possible", but doing so is a conscious decision.

As an alternative, I think we can either make the existing FromWebFrame method
accept a const WebFrame and return a non-const RenderFrame (using the const_cast
code you already added), or else just make the callers provide a non-const
WebFrame.

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:70: virtual int GetRoutingID() const = 0;
Per the above, const methods in the content/public interfaces are forbidden.

https://codereview.chromium.org/1373573002/diff/120001/content/renderer/dom_s...
File content/renderer/dom_serializer_browsertest.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/renderer/dom_s...
content/renderer/dom_serializer_browsertest.cc:166:
command_line->AppendSwitch(switches::kSingleProcess);
[FYI comment, no action required] When you write new tests in the future, know
that there are good alternatives to the pattern used here. Here, the whole
renderer stack is made to run in the browser process. That results in a .cc file
that #includes both "content/public/renderer" and "content/public/browser",
which is very strange and, outside of a test that works like this, totally
nonsensical.

A more conventional way to write this test (at least nowadays) would be to make
it a RenderViewTest, where test code runs in a renderer environment, there's no
browser process at all, and the IPC interactions with the browser process are
mocked out and validated.

It's totally fine, of course, to update/maintain the existing test. Just wanted
to let you know that this is a weird pattern, and probably not a great idea for
future tests.

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializerImpl.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:
WebPageSerializerImpl(WebFrame*, WebPageSerializerClient*);
Why drop the parameter names -- is that just The Blink Way? 

blink code is outside of my stylistic expertise, but to my chromium-colored
eyes, it seems like removing the parameter names and having the comments refer
to the params by position makes this harder to understand than before.

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializerClient.h:55: virtual void
writeLocalPathForSubFrame(const WebFrame& subFrame) = 0;
It seems more common for the F in subFrame to not be capitalized (grepping the
inside webkit c++ code, it's about 55 subframe vs. 4 subFrame. layout tests tend
to say 'subframe' too by an even wider margin)

[ncarter (slow), 2015-10-08 21:04:14]

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1168: attr_value = fallback_url.spec();
When does this happen? Could you add a comment explaining why we need it?

[Łukasz Anforowicz, 2015-10-09 16:54:08]

Nick, can you take another look please?

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:166:
contents_id_(web_contents->GetRenderProcessHost()->GetID()),
On 2015/10/08 20:57:54, ncarter wrote:
> Not introduced by your cl, but just wanted to put it on your radar: This is
> yucky, since two WC's can easily share a RPH. If you've got cycles to fix it
(in
> a separate patch), I might suggest using the FTN id of the root FrameTreeNode
of
> the WebContents. It ought to be globally unique and stable over the lifetime
of
> a tab/WebContents.

Sure - I'll put that on my TODO list.  OTOH, I see that
SavePackage::content_id() is passed to SaveFileManager and referred to later on
as render_process_id + not used as a key in SaveFileManager's maps (so maybe
this is just a naming issue / maybe this doesn't need to be unique).

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1004: bool handled = true;
On 2015/10/08 20:57:55, ncarter wrote:
> Consider adding here:
> 
> RenderFrameHostImpl* sender =
> static_cast<RenderFrameHostImpl*>(render_frame_host);
> 
> And using that as the argument to WITH_PARAM. This would allow your handler
> methods to accept an RFHImpl (if needed), and might let you get rid of helpers
> like GetFrameTreeNodeId.

Done.  Thanks for the suggestion.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1048: DCHECK_LT(0,
number_of_frames_pending_response_);
On 2015/10/08 20:57:54, ncarter wrote:
> Another "fyi, no action needed, just want to put this on your radar" comment: 
> 
> This DCHECK (which this CL doesn't add) is valid because we don't skip over
> crashed subframes or mainframes. You can see this in practice: navigate to
> "about:crash" to get a sad tab, and hit Ctrl+S. This creates a save page
> operation that'll never terminate.
> 
> That's probably not a very serious bug (I think the URL-based deduping issues
> you've uncovered are way more serious, for example). But it does seem somewhat
> plausible to me, that someone would want to save-page something that has a
> crashed subframe.

Disclaimer: I've introduced this DCHECK in an earlier CL (to explicitly document
that the code assumes that at this point we are waiting for a response from at
least 1 renderer process).

I think handling of missing IPC responses is a problem that might need a broader
/ generic solution.  MHTML [1] tries to detect renderer crashes, but doesn't
consider timeouts [2].  SavePage doesn't account for missing IPC responses at
all.  Ideally both should be able to reuse some kind of a code pattern to
account for slow or crashing renderers.

Also note that a renderer might be killed after we send the IPC, but before it
sends back a response (although I guess one can try to dismiss this as an even
rarer occurrence than a use case of saving a page with an *already* crashed
subframe).

FWIW, I've opened crbug.com/541342 (after testing that the problem occurs before
my changes).

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

[2] OTOH, maybe we don't need to worry about timeouts - there is a separate
check of whether a renderer is regularily sending IPCs and user is given an
option of killing the renderer when it is slow.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1064: SaveItem* save_item =
frame_tree_node_id_to_save_item_[frame_tree_node_id];
On 2015/10/08 20:57:55, ncarter wrote:
> What if it's not in the map?

Good point.  Thanks for catching this.

When thinking how to handle this, I've considered for a moment having a check in
GetSerializedHtmlWithLocalLinksForFrame to see if the frame is present in the
map.  OTOH, this wouldn't have helped in all the cases (i.e. when getting an IPC
from a misbehaving renderer), so I think we should just return here and avoid
complicating GetSerializedHtmlWithLocalLinksForFrame.

Also - I've opened crbug.com/541354 to explicitly track the raciness.

Done?  WDYT?

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1093: ->GetFrameTree()
On 2015/10/08 20:57:54, ncarter wrote:
> If you passed in an RFHImpl here, you could alternatively get this via
> sender->frame_tree_node()->frame_tree(). There's actually a subtle difference
> between the two: when an interstitial is showing, it creates a second frame
tree
> for its content, so in code that needs to work with interstitial RFH's, going
to
> the WebContents would be a mistake. For save page, the difference doesn't
> matter, since save-page seems to be disabled while an interstitial is showing.

Done (to have a safer code in case somebody wants to base his/her code on this).

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1097: return;
On 2015/10/08 20:57:55, ncarter wrote:
> Would it be worthwhile here to validate that frame_tree_node->parent() ==
> sender->frame_tree_node()? 

Done.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1100:
frame_tree_node_id_to_save_item_[frame_tree_node->frame_tree_node_id()];
On 2015/10/08 20:57:55, ncarter wrote:
> What if it's not in the map?

It's ok in this case - this will be handled by SerializeLocalPathForSaveItem
(which will use the fallback url in this case).

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1138:
number_of_frames_pending_response_--;
On 2015/10/08 20:57:55, ncarter wrote:
> Shouldn't save_item()->state() be transitioned to something else here (as a
> defense against OnSerializedEndOfFrame being sent multiple times for a frame)?
> 
> When I'm looking at browser-process code like this, I'm always asking the
> question: could an attacker-controlled renderer process get me to do something
I
> didn't intend here? One broad defense for that is to avoid processing a
response
> message if we can't match it up to a request.

I've spent 10-15 minutes thinking that SaveItem::state_ might be getting
accessed outside of the browser UI thread, but couldn't find any such case. 
I've added DCHECKs to this point.

I guess it makes sense to add a COMPLETING state.

Done (I think).

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1168: attr_value = fallback_url.spec();
On 2015/10/08 21:04:14, ncarter wrote:
> When does this happen? Could you add a comment explaining why we need it?

Done.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1171: // TODO(lukasza): Escape
attr_value (&quot; + &amp;).
On 2015/10/08 20:57:55, ncarter wrote:
> My understanding is that there are no security considerations here, really,
> since the renderer can by design write arbitrary content to the file.
> 
> But escaping could matter for correctness, if URL-important characters (like #
> or ?) or context-important characters (like ") were to find their way into
> saved_main_directory_path_ or save_item->file_name(). So, what kind of
escaping
> needs to happen here, exactly? Should we pass it through GURL for
> canonicalization?
> 
> I'm OK with leaving this as a TODO, provided a CL to address it is right
around
> the corner. Doubly okay if that CL includes unittests that exercise the weird
> cases (filename-reserved chars, url chars, PRN, COM9, etc). Over-escaping is
> just as likely to be a cause of trouble as under-escaping, and it seems like
> there's already some canonicalization code in place.

Hmmm... thinking about it some more, I've realized that this code is not
landable for 2 reasons:

1. It also handles fallback url (which doesn't go through filename sanitization)

2. This doesn't do encoding equivalent to
WebPageSerializerImpl::encodeAndFlushBuffer:

2a. I am not quite sure what will happen to characters outside of ASCII /
whether they will match file encoding.

2b. I see that encoding from #2 somehow handles quotes (not sure yet how, but
they get escaped via %).  Interestingly #2 still doesn't handle amperstands.

Let's get the other code review feedback out of the way and I'll work on this
issue next...

More notes:

- Not sure if the browser has any html escaping utilities - last time I checked
most utilities of this kind were in the blink layer...

- We probably need to keep in mind that if the escaping ends up being more
complicated then it will be an additional attack surface the renderer can
exploit...

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1233: if (save_item == nullptr) {
On 2015/10/08 20:57:55, ncarter wrote:
> You seem to be preferring operator[] throughout instead of map::find(), which
> means that a failed lookup can wind up with a nullptr getting into the map. 
Why
> is that? With the auto keyword, you can do it pretty concisely, and for
> readability, you can store it->second into a SaveItem* variable once you've
> verified it != map.end().
> 
> Here, even though the null value is replaced a couple lines later, I did a
> double-take before realizing that's what was going on -- it's not a pattern
I'm
> used to seeing.

I've changed to find in some places, but when I tried this in this case , it led
to more complicated code (because we want to call set_referrer when the item is
already present *and* when it is missing).  So I think I'll just leave the code
as-is (since it won't leave weird nullptr entries in the map [unlike the other
places in this cc file]).

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1269: std::sort(
On 2015/10/08 20:57:55, ncarter wrote:
> I wonder if we ought to use stable_sort here to avoid a possible source of
> nondeterminism / cross-platform differences? (this assumes, of course, that
> waiting_item_queue_ is already in some kind of relative tree-order, which may
> not be the case at all? If the order is random going in, sort() won't make it
> worse)

There is a partial order here (things from the same frame are next to each other
+ things from the same order are in the order they were reported by the frame). 
So it probably makes sense to preserve some if it via stable_sort.

Done.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1275: (y->save_source() ==
SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
On 2015/10/08 20:57:55, ncarter wrote:
> Would this be easier to comprehend if we wrote it as:
> 
> x != y && y == SAVE_FILE_FROM_DOM ?
> 
> (I'm honestly not sure. I was confused at first by your version, but once I
> figured it out, it looks obvious. Leaving it up to you.)

I think your comparison would be broken as it would say "false" for both x<y and
y<x if both x and y were SAVE_FILE_FROM_DOM.

At the same time, you have a point.  I am not sure if it helps, but I've added a
comment clarifying what it means that frames go last.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.h:196: // "blanks" that we can replace
with links to local paths).
On 2015/10/08 20:57:55, ncarter wrote:
> Make this parenthetical clause its own sentence, something like: Links will be
> represented as "blanks", which we'll replace with local paths later.
> 
> Also: is "blanks" the right term to use? I don't see it used anywhere else in
> save_package.cc.

I guess this comment is confusing (I have a feeling that you might have
misunderstood what it was trying to say - there are no actual blanks in the
serialized html - there are just OnSerializedLocalPathForFoo callbacks).  I
think removing the parenthetical clause is the best thing to do.

https://codereview.chromium.org/1373573002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:1182:
IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForSubFrame,
On 2015/10/08 20:57:55, ncarter wrote:
> I think ChildFrame is a better term than SubFrame for this context, to be
> consistent with FrameHostMsg_CreateChildFrame and
FrameMsg_SetChildFrameSurface.
> 
> If you do rename this, don't forget to update the instance in the (awesome) CL
> description too.

Done.  (FWIW I assume only frame_messages.h (+fallout) need changing based on
your acking of subframe in
https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/pub...)

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:60: static const RenderFrame*
FromWebFrame(const blink::WebFrame* web_frame);
On 2015/10/08 20:57:55, ncarter wrote:
> I appreciate the justification you gave for adding these const overrides in
the
> CL description, but for reasons I'll explain below, we shouldn't need them.
The
> TL;DR is that "const RenderFrame*" ought to be useless.
> 
> The rules we try to enforce for the content api (which this, being
> content/public, is part of) are posted here:
> http://www.chromium.org/developers/content-module/content-api. One of them
says:
> "Don't add the const identifier to interfaces. ... For interfaces implemented
by
> content, the implementation details doesn't have to be exposed."  Per that
> policy, all the methods of RenderFrame ought to be non-const, and it's common
> for RenderFrame's const-hostility to spilling over to code that has to
interacts
> with it. Although there are a couple const methods today, they're in violation
> of the policy and we'd like to eradicate them too.
> 
> Yes, this goes against the Effective C++ recommendation of "use const wherever
> possible", but doing so is a conscious decision.
> 
> As an alternative, I think we can either make the existing FromWebFrame method
> accept a const WebFrame and return a non-const RenderFrame (using the
const_cast
> code you already added), or else just make the callers provide a non-const
> WebFrame. 

Done:

- Removed const-related changes

- Added a GetRoutingIdForFrameOrProxy overload that takes
  a const WebFrame* (with a comment stating why const_cast
  there is safe).

https://codereview.chromium.org/1373573002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:70: virtual int GetRoutingID() const = 0;
On 2015/10/08 20:57:55, ncarter wrote:
> Per the above, const methods in the content/public interfaces are forbidden.

Done.

https://codereview.chromium.org/1373573002/diff/120001/content/renderer/dom_s...
File content/renderer/dom_serializer_browsertest.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/renderer/dom_s...
content/renderer/dom_serializer_browsertest.cc:166:
command_line->AppendSwitch(switches::kSingleProcess);
On 2015/10/08 20:57:55, ncarter wrote:
> [FYI comment, no action required] When you write new tests in the future, know
> that there are good alternatives to the pattern used here. Here, the whole
> renderer stack is made to run in the browser process. That results in a .cc
file
> that #includes both "content/public/renderer" and "content/public/browser",
> which is very strange and, outside of a test that works like this, totally
> nonsensical.
> 
> A more conventional way to write this test (at least nowadays) would be to
make
> it a RenderViewTest, where test code runs in a renderer environment, there's
no
> browser process at all, and the IPC interactions with the browser process are
> mocked out and validated.
> 
> It's totally fine, of course, to update/maintain the existing test. Just
wanted
> to let you know that this is a weird pattern, and probably not a great idea
for
> future tests.
> 

Acknowledged.

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializerImpl.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:
WebPageSerializerImpl(WebFrame*, WebPageSerializerClient*);
On 2015/10/08 20:57:55, ncarter wrote:
> Why drop the parameter names -- is that just The Blink Way? 
> 
> blink code is outside of my stylistic expertise, but to my chromium-colored
> eyes, it seems like removing the parameter names and having the comments refer
> to the params by position makes this harder to understand than before.

Yes - this is blink-ism.  The error method from presubmit checks is:

** Presubmit ERRORS **
check-webkit-style failed
third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:  The parameter name
"client" adds no information, so it should be removed. 
[readability/parameter_name] [5]

Yes - I also find documentation comments in blink world weird, when they cannot
use parameter names when documenting them.  Maybe something worth raising on
chromium-dev?

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializerClient.h:55: virtual void
writeLocalPathForSubFrame(const WebFrame& subFrame) = 0;
On 2015/10/08 20:57:55, ncarter wrote:
> It seems more common for the F in subFrame to not be capitalized (grepping the
> inside webkit c++ code, it's about 55 subframe vs. 4 subFrame. layout tests
tend
> to say 'subframe' too by an even wider margin)
> 

Done:

- In my changes

- In dom_serializer_browsertest.cc (the only place outside of changes that hit
'[sS]ubFrame' regex in content).  Under WebKit there are multiple cases with
uppercase F, so I only modified my changes.

[dcheng, 2015-10-09 16:59:33]

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializerImpl.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:
WebPageSerializerImpl(WebFrame*, WebPageSerializerClient*);
On 2015/10/09 at 16:54:08, Łukasz Anforowicz wrote:
> On 2015/10/08 20:57:55, ncarter wrote:
> > Why drop the parameter names -- is that just The Blink Way? 
> > 
> > blink code is outside of my stylistic expertise, but to my chromium-colored
> > eyes, it seems like removing the parameter names and having the comments
refer
> > to the params by position makes this harder to understand than before.
> 
> Yes - this is blink-ism.  The error method from presubmit checks is:
> 
> ** Presubmit ERRORS **
> check-webkit-style failed
> third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:  The parameter name
"client" adds no information, so it should be removed. 
[readability/parameter_name] [5]
> 
> Yes - I also find documentation comments in blink world weird, when they
cannot use parameter names when documenting them.  Maybe something worth raising
on chromium-dev?

Usually, we don't document parameters by position. I'd just update this comment
to say:
Creates a serializer to serialize the given frame, sending the data to a client
sink interface to consume.

[ncarter (slow), 2015-10-12 23:31:48]

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:166:
contents_id_(web_contents->GetRenderProcessHost()->GetID()),
On 2015/10/09 16:54:08, Łukasz Anforowicz wrote:
> On 2015/10/08 20:57:54, ncarter wrote:
> > Not introduced by your cl, but just wanted to put it on your radar: This is
> > yucky, since two WC's can easily share a RPH. If you've got cycles to fix it
> (in
> > a separate patch), I might suggest using the FTN id of the root
FrameTreeNode
> of
> > the WebContents. It ought to be globally unique and stable over the lifetime
> of
> > a tab/WebContents.
> 
> Sure - I'll put that on my TODO list.  OTOH, I see that
> SavePackage::content_id() is passed to SaveFileManager and referred to later
on
> as render_process_id + not used as a key in SaveFileManager's maps (so maybe
> this is just a naming issue / maybe this doesn't need to be unique).

Looks like it's used as a key in a map here, so there's at least a theoretical
bug here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...

I'm not super worried about the implications of the bug, but it would be nice to
fix up while we're modernizing this code.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1048: DCHECK_LT(0,
number_of_frames_pending_response_);
On 2015/10/09 16:54:07, Łukasz Anforowicz wrote:
> On 2015/10/08 20:57:54, ncarter wrote:
> > Another "fyi, no action needed, just want to put this on your radar"
comment: 
> > 
> > This DCHECK (which this CL doesn't add) is valid because we don't skip over
> > crashed subframes or mainframes. You can see this in practice: navigate to
> > "about:crash" to get a sad tab, and hit Ctrl+S. This creates a save page
> > operation that'll never terminate.
> > 
> > That's probably not a very serious bug (I think the URL-based deduping
issues
> > you've uncovered are way more serious, for example). But it does seem
somewhat
> > plausible to me, that someone would want to save-page something that has a
> > crashed subframe.
> 
> Disclaimer: I've introduced this DCHECK in an earlier CL (to explicitly
document
> that the code assumes that at this point we are waiting for a response from at
> least 1 renderer process).
> 
> I think handling of missing IPC responses is a problem that might need a
broader
> / generic solution.  MHTML [1] tries to detect renderer crashes, but doesn't
> consider timeouts [2].  SavePage doesn't account for missing IPC responses at
> all.  Ideally both should be able to reuse some kind of a code pattern to
> account for slow or crashing renderers.
> 
> Also note that a renderer might be killed after we send the IPC, but before it
> sends back a response (although I guess one can try to dismiss this as an even
> rarer occurrence than a use case of saving a page with an *already* crashed
> subframe).
> 
> FWIW, I've opened crbug.com/541342 (after testing that the problem occurs
before
> my changes).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
> 
> [2] OTOH, maybe we don't need to worry about timeouts - there is a separate
> check of whether a renderer is regularily sending IPCs and user is given an
> option of killing the renderer when it is slow.

I agree with everything in your reply. I don't think we should bother with
timeouts or anything to be resilient against unresponsive renderers: the hang
monitor watchdog is the browser-wide solution for that.

Engineering effort around crashed and hung renderers should be limited to the
following: (a) never hang or jank the main browser UI, even if we're stuck
waiting for a reply that never comes (b) cancellation of operations should work
even in the presence of unresponsive renderers (c) we don't leak any
browser-side resources if a renderer crashes mid-operation.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1138:
number_of_frames_pending_response_--;
On 2015/10/09 16:54:08, Łukasz Anforowicz wrote:
> On 2015/10/08 20:57:55, ncarter wrote:
> > Shouldn't save_item()->state() be transitioned to something else here (as a
> > defense against OnSerializedEndOfFrame being sent multiple times for a
frame)?
> > 
> > When I'm looking at browser-process code like this, I'm always asking the
> > question: could an attacker-controlled renderer process get me to do
something
> I
> > didn't intend here? One broad defense for that is to avoid processing a
> response
> > message if we can't match it up to a request.
> 
> I've spent 10-15 minutes thinking that SaveItem::state_ might be getting
> accessed outside of the browser UI thread, but couldn't find any such case. 
> I've added DCHECKs to this point.
> 
> I guess it makes sense to add a COMPLETING state.
> 
> Done (I think).

Gosh, I hope not. I'm glad you added the UI thread DCHECKs in addition to the
COMPLETING state.

https://codereview.chromium.org/1373573002/diff/120001/content/browser/downlo...
content/browser/download/save_package.cc:1233: if (save_item == nullptr) {
On 2015/10/09 16:54:07, Łukasz Anforowicz wrote:
> On 2015/10/08 20:57:55, ncarter wrote:
> > You seem to be preferring operator[] throughout instead of map::find(),
which
> > means that a failed lookup can wind up with a nullptr getting into the map. 
> Why
> > is that? With the auto keyword, you can do it pretty concisely, and for
> > readability, you can store it->second into a SaveItem* variable once you've
> > verified it != map.end().
> > 
> > Here, even though the null value is replaced a couple lines later, I did a
> > double-take before realizing that's what was going on -- it's not a pattern
> I'm
> > used to seeing.
> 
> I've changed to find in some places, but when I tried this in this case , it
led
> to more complicated code (because we want to call set_referrer when the item
is
> already present *and* when it is missing).  So I think I'll just leave the
code
> as-is (since it won't leave weird nullptr entries in the map [unlike the other
> places in this cc file]).

I'm OK with operator[] in these cases, since it's safe. But FWIW, if you wanted
to use iterators, the pattern I'd use is:

auto it = url_to_save_item_.find(u);
if (it == url_to_save_item_.end()) {
  it = map.insert(it, std::make_pair(u, new SaveItem(..)));
}
SaveItem* save_item = it->second;
save_item->set_referrer(...);

https://codereview.chromium.org/1373573002/diff/180001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/180001/content/browser/downlo...
content/browser/download/save_package.cc:1188: // TODO(lukasza): Escape
attr_value (" + &).
Responding to your comments in the earlier thread, three thoughts:

==

I don't see this as being worrisome attack surface for a sandbox escape (since
the attacker can write whatever they want to the file already), but it might be
an interesting target somehow from the web, to the extent that a saved-page can
insert """>;}"'/>"""LPT1 or something into what becomes the saved the filename.
We definitely need to plug the latter kind of hole as part of this CL, since
we're losing the protection offered by blink.

==

I don't see why we need any HTML escaping at all here?

The renderer has written everything up to and including the quotation mark, say:

<img src="

And we have to write something after that that's a legal URL, that maps to the
filename, and that doesn't break out of the context?, right? I think URL
canonicalization (via GURL, or maybe there's another util) will do that.

In other words, quotation marks should become %22, not &quot; -- right?

==

It looks like there might be some utils in net for going between file-scheme
URLs and FilePaths, but I'm not sure if they help here or not.

[Łukasz Anforowicz, 2015-10-21 22:50:35]

I am publishing replies to 2 CR comments addressed in recent patchsets (mostly
to get this CR out of the "issues with drafts by me" list in my Rietveld UI),
but please hold off with looking at the CL just yet - this CL will be most
likely abandoned and replaced with crrev.com/1414453007

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializerImpl.h (right):

https://codereview.chromium.org/1373573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:
WebPageSerializerImpl(WebFrame*, WebPageSerializerClient*);
On 2015/10/09 16:59:33, dcheng wrote:
> On 2015/10/09 at 16:54:08, Łukasz Anforowicz wrote:
> > On 2015/10/08 20:57:55, ncarter wrote:
> > > Why drop the parameter names -- is that just The Blink Way? 
> > > 
> > > blink code is outside of my stylistic expertise, but to my
chromium-colored
> > > eyes, it seems like removing the parameter names and having the comments
> refer
> > > to the params by position makes this harder to understand than before.
> > 
> > Yes - this is blink-ism.  The error method from presubmit checks is:
> > 
> > ** Presubmit ERRORS **
> > check-webkit-style failed
> > third_party/WebKit/Source/web/WebPageSerializerImpl.h:76:  The parameter
name
> "client" adds no information, so it should be removed. 
> [readability/parameter_name] [5]
> > 
> > Yes - I also find documentation comments in blink world weird, when they
> cannot use parameter names when documenting them.  Maybe something worth
raising
> on chromium-dev?
> 
> Usually, we don't document parameters by position. I'd just update this
comment
> to say:
> Creates a serializer to serialize the given frame, sending the data to a
client
> sink interface to consume.

Done.

https://codereview.chromium.org/1373573002/diff/180001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1373573002/diff/180001/content/browser/downlo...
content/browser/download/save_package.cc:1188: // TODO(lukasza): Escape
attr_value (" + &).
On 2015/10/12 23:31:48, ncarter wrote:
> Responding to your comments in the earlier thread, three thoughts:
> 
> ==
> 
> I don't see this as being worrisome attack surface for a sandbox escape (since
> the attacker can write whatever they want to the file already), but it might
be
> an interesting target somehow from the web, to the extent that a saved-page
can
> insert """>;}"'/>"""LPT1 or something into what becomes the saved the
filename.
> We definitely need to plug the latter kind of hole as part of this CL, since
> we're losing the protection offered by blink.
> 
> ==
> 
> I don't see why we need any HTML escaping at all here?
> 
> The renderer has written everything up to and including the quotation mark,
say:
> 
> <img src="
> 
> And we have to write something after that that's a legal URL, that maps to the
> filename, and that doesn't break out of the context?, right? I think URL
> canonicalization (via GURL, or maybe there's another util) will do that.
> 
> In other words, quotation marks should become %22, not &quot; -- right?
> 
> ==
> 
> It looks like there might be some utils in net for going between file-scheme
> URLs and FilePaths, but I'm not sure if they help here or not.

Daniel pointed out net::EscapeForHTML.  I am not sure how I could have missed it
earlier.  Works great.

[Łukasz Anforowicz, 2015-10-28 22:33:04]

Description was changed from

==========
OOPIFs: Moving stitching of local paths from renderer to browser process.

Context
=======

Save-Page-As in complete-html mode replaces links to subframes and
savable resources (i.e. img elements) with paths to locally saved files.
This replacement used to be done in the renderer process, but has to be
moved to the browser process, because with OOP iframes renderer process
is not able to look into any data belonging to subframes living in
another process (and "any data" covers URLs of subframes or lists of
resources contained in subframes).

Before this CL, saving (as complete html) a page with cross-site iframes in
OOPIFs mode would crash the renderer process.  After this CL there is no crash
(and the test in save_page_browsertest.cc should prevent regressions).


IPC Messages
============

This CL replaces the following 2 IPC messages:

    IPC_MESSAGE_ROUTED3(FrameMsg_GetSerializedHtmlWithLocalLinks,
                        std::vector<GURL> /* urls that have local copy */,
                        std::vector<base::FilePath> /* paths of local copy */,
                        base::FilePath /* local directory path */)

    IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializedHtmlWithLocalLinksResponse,
                        GURL /* frame URL */,
                        std::string /* data buffer */,
                        int32 /* complete status */)

with the following 5 IPC messages:

    IPC_MESSAGE_ROUTED0(FrameMsg_GetSerializedHtmlWithLocalLinks);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedHtmlFragment,
                        std::string /* data buffer */);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForChildFrame,
                        int /* RenderFrame's or RenderFrameProxy's routing id
                               for the child frame we need a local path for.*/);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForSavableResource,
                        GURL /* savable resource link */);

    IPC_MESSAGE_ROUTED0(FrameHostMsg_SerializedEndOfFrame)

Notice that
- Urls and local paths are no longer passed from browser to renderer
  processes.
- Renderer responses have been split into more granular responses (html
  fragment, local path for subresource, local path for child frame, end
  of data).

All the other changes in the CL are a consequence of the IPC change outlined
above.


Other notes
===========

Mapping frames and subframes to SaveItems is done via frame_tree_node_id.

Note that the old WebPageSerializerImpl code [1] did *not* escape file paths
embedded in serializer output as html attribute values.  The current CL
preserves this behavior (in SavePackage::SerializeLocalPathForSaveItem).

[1]
https://chromium.googlesource.com/chromium/blink/+/9d8944796d0b1208175177cda8...


BUG=526786
==========

to

==========
ABANDONED CL - REPLACED WITH crrev.com/1413103003

OOPIFs: Moving stitching of local paths from renderer to browser process.

Context
=======

Save-Page-As in complete-html mode replaces links to subframes and
savable resources (i.e. img elements) with paths to locally saved files.
This replacement used to be done in the renderer process, but has to be
moved to the browser process, because with OOP iframes renderer process
is not able to look into any data belonging to subframes living in
another process (and "any data" covers URLs of subframes or lists of
resources contained in subframes).

Before this CL, saving (as complete html) a page with cross-site iframes in
OOPIFs mode would crash the renderer process.  After this CL there is no crash
(and the test in save_page_browsertest.cc should prevent regressions).


IPC Messages
============

This CL replaces the following 2 IPC messages:

    IPC_MESSAGE_ROUTED3(FrameMsg_GetSerializedHtmlWithLocalLinks,
                        std::vector<GURL> /* urls that have local copy */,
                        std::vector<base::FilePath> /* paths of local copy */,
                        base::FilePath /* local directory path */)

    IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializedHtmlWithLocalLinksResponse,
                        GURL /* frame URL */,
                        std::string /* data buffer */,
                        int32 /* complete status */)

with the following 5 IPC messages:

    IPC_MESSAGE_ROUTED0(FrameMsg_GetSerializedHtmlWithLocalLinks);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedHtmlFragment,
                        std::string /* data buffer */);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForChildFrame,
                        int /* RenderFrame's or RenderFrameProxy's routing id
                               for the child frame we need a local path for.*/);

    IPC_MESSAGE_ROUTED1(FrameHostMsg_SerializedLocalPathForSavableResource,
                        GURL /* savable resource link */);

    IPC_MESSAGE_ROUTED0(FrameHostMsg_SerializedEndOfFrame)

Notice that
- Urls and local paths are no longer passed from browser to renderer
  processes.
- Renderer responses have been split into more granular responses (html
  fragment, local path for subresource, local path for child frame, end
  of data).

All the other changes in the CL are a consequence of the IPC change outlined
above.


Other notes
===========

Mapping frames and subframes to SaveItems is done via frame_tree_node_id.

Note that the old WebPageSerializerImpl code [1] did *not* escape file paths
embedded in serializer output as html attribute values.  The current CL
preserves this behavior (in SavePackage::SerializeLocalPathForSaveItem).

[1]
https://chromium.googlesource.com/chromium/blink/+/9d8944796d0b1208175177cda8...


BUG=526786
==========