Workarounding fix for save complete page problems.

Workarounding fix for save complete page problems.

BUG=32771
This change is something between a real fix and a workaround in strict
sense. The current save complete page architecture doesn't handle the cases
of duplicate urls of the serializable resources, these are typically the do
cuments - main one and the ones in iframes. The most severe symptom of the
flaw is when an iframe has the same url as the main document. This is
possible in a case when a document in the iframe is filled by a js
document.write calls from the parent's document script(s). And there is no
direct url loading in that iframe - e.g. by setting src attribute or by
location DOM object of the iframe's window. In such case the main index
file might not be saved, because an inappropriate sequence of actions were
carried out - like save finished, write another data to already finished
file.

The fact that the architecture is flawed is also visible in a way
the save page data is received from the renderer. First we get the list of
serializable urls with no information about the number of occurences for
each. And then when the data is received, we might get separate contents
for each occurence of the given url.

Knowing all above, the simple workaround is to prevent writing any more
data when the file is already finished. This way we don't regress anything
- in worst case the saved page will be incomplete. But there are plenty
other possiblities it will be incomplete, because of more complex scripts
that are generating content. Also, since index file is serialized as first,
we won't suffer the case of not saving it and it is the most important file
naturally. The complete fix would require a big task that would actually
improve the whole save complete page functionality, so I simply elevated
the workaround to a workarounding fix that should be sufficient as it fixes
most cases and doesn't regress anything.

With this change, saving the complete pages with duplicate urls among
iframes and main doc should be considered successful in the
SavePackage::wait_state_ sense, but the fact of the url duplication will
be noted when trying to write more data to already finished file.

[pdamek, 2014-04-10 20:02:27]

That is what I managed to do about
https://code.google.com/p/chromium/issues/detail?id=32771

[asanka, 2014-04-14 23:35:22]

On 2014/04/10 20:02:27, pdamek wrote:
> That is what I managed to do about
> https://code.google.com/p/chromium/issues/detail?id=32771

Thanks for the patch. It looks like you are trying to fix
https://code.google.com/p/chromium/issues/detail?id=106364 instead of 32771,
which has a reduced test case very similar to the one you are submitting.

You are correct that there being multiple instances of the same URL is causing
problems. I'm not sure what the correct fix is, but my inclination is for the
renderer to stop sending multiple resources for the same URL. I'll update this
CL once I have a better idea of how to do this. My preference is for us to
address the problem rather than submit a workaround.

[pdamek, 2014-04-15 13:27:13]

On 2014/04/14 23:35:22, asanka wrote:
> On 2014/04/10 20:02:27, pdamek wrote:
> > That is what I managed to do about
> > https://code.google.com/p/chromium/issues/detail?id=32771
> 
> Thanks for the patch. It looks like you are trying to fix
> https://code.google.com/p/chromium/issues/detail?id=106364 instead of 32771,
> which has a reduced test case very similar to the one you are submitting.
> 

Actually, I'll insist that this "workaround" is more connected with 32771 than
106364. That is because I was aware of 106364 existence. For various reasons
during my work I decided to try to do as little change as possible.

> You are correct that there being multiple instances of the same URL is causing
> problems. I'm not sure what the correct fix is, but my inclination is for the
> renderer to stop sending multiple resources for the same URL. I'll update this
> CL once I have a better idea of how to do this. My preference is for us to
> address the problem rather than submit a workaround.

I'm perfectly aware that making a full fix would be better, however I disagree
that in this case that I didn't address the problem at all. See the whole long
description. Despite the preference, I hope it is possible to accept this patch
(with some minor corrections possibly). Briefly the reasons are following:

1) The bug is 4 years old and nobody has had a time to fix it properly.
2) I have spent some significant time on this work and unfortunatelly it would
be problematic for me to spend more, because this bug has not enough priority
for us (@ Opera). Actually I started working on it, because it was given too
high priority at first by accident :). So bearing that in mind at some point I
wanted to finish the work quickly, but didn't want the investigation to be
wasted totally.
3) The totally proper fix would need some really big effort. As I mentioned in a
description, the current save page mechanism is very flawed when there is lots
of script/dynamically generated content. For example all in script coded
resources urls are not properly changed to local paths.

I tried to do this workaround as clean as possible. I hope it can be accepted
and then you can make a real fix in e.g. half a year time or sth :).

[asanka, 2014-04-15 20:10:43]

+rdsmith: FHI

On 2014/04/15 13:27:13, pdamek wrote:
> On 2014/04/14 23:35:22, asanka wrote:
> > On 2014/04/10 20:02:27, pdamek wrote:
> > > That is what I managed to do about
> > > https://code.google.com/p/chromium/issues/detail?id=32771
> > 
> > Thanks for the patch. It looks like you are trying to fix
> > https://code.google.com/p/chromium/issues/detail?id=106364 instead of 32771,
> > which has a reduced test case very similar to the one you are submitting.
> > 
> 
> Actually, I'll insist that this "workaround" is more connected with 32771 than
> 106364. That is because I was aware of 106364 existence. For various reasons
> during my work I decided to try to do as little change as possible.

The only aspect this CL deals with is the prospect of multiple resources being
associated with the same URL. Which is why I referred to issue 106364. To
resolve issue 32771 we'd need to have better handling of failure cases (due to
URL collisions or disk errors or others) such that the browser doesn't end up in
the semi finished state described in the bug.

> > You are correct that there being multiple instances of the same URL is
causing
> > problems. I'm not sure what the correct fix is, but my inclination is for
the
> > renderer to stop sending multiple resources for the same URL. I'll update
this
> > CL once I have a better idea of how to do this. My preference is for us to
> > address the problem rather than submit a workaround.
> 
> I'm perfectly aware that making a full fix would be better, however I disagree
> that in this case that I didn't address the problem at all. See the whole long
> description. Despite the preference, I hope it is possible to accept this
patch
> (with some minor corrections possibly). Briefly the reasons are following:
> 
> 1) The bug is 4 years old and nobody has had a time to fix it properly.
> 2) I have spent some significant time on this work and unfortunatelly it would
> be problematic for me to spend more, because this bug has not enough priority
> for us (@ Opera). Actually I started working on it, because it was given too
> high priority at first by accident :). So bearing that in mind at some point I
> wanted to finish the work quickly, but didn't want the investigation to be
> wasted totally.
> 3) The totally proper fix would need some really big effort. As I mentioned in
a
> description, the current save page mechanism is very flawed when there is lots
> of script/dynamically generated content. For example all in script coded
> resources urls are not properly changed to local paths.

document.open() sets the target document's url and origin to be equal to that of
the owning document. Are there other cases? 

> I tried to do this workaround as clean as possible. I hope it can be accepted
> and then you can make a real fix in e.g. half a year time or sth :).

I appreciate your effort on this CL so far. This is indeed an old bug. My
suggestion was that we figure out what the correct behavior should be rather
than dismiss it as being too much effort. I'm happy with an interim fix as long
as one is reliably better than what's there now and doesn't negatively affect
code health.

[Randy Smith (Not in Mondays), 2014-04-15 20:47:52]

My perspective on this type of change is that user experience comes first, with
code health a close second, and that with a project like Chromium, it's
depressingly easy to regress on both those metrics if we aren't careful.

If we were sure that this would improve the user experience for the majority of
users, I'd be in favor of doing some variant of it even though it's a workaround
rather than a real fix (though I'd want to clean it up some; I have a personal
hobbyhorse around adding booleans to a class to represent variants on core
state).  But I don't think we understand the universe of possible failure modes
well enough to understand what overall effect this would have.  Maybe in those
cases when a file is written to twice, the second write is more often the
correct one?  Maybe the majority of cases have interleaved writes that leave the
file completely garbaged, and stopping after the first one finishes won't help? 


Normally I'd say something like "let's add some UMA and gather that data!"  But
I suspect in this case that would be as much if not more work as actually fixing
the problem :-{.   So, with apologies for time wasted, if you don't have time to
do a complete fix, I'd recommend dropping this CL.  At least the work helped you
learn the code better? :-J

[pdamek, 2014-04-16 19:30:21]

> 
> document.open() sets the target document's url and origin to be equal to that
of
> the owning document. Are there other cases? 

Yes, consider the following:

index.htm:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
</head>
<body>
<iframe id="iframe1" src="iframe.htm"></iframe>
<iframe id="iframe2" src="iframe.htm"></iframe>
</body>
</html>

iframe.htm:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
<script type="text/javascript">

function dummy() {
   document.body.innerHTML = "more dummy";
};

</script>
</head>
<body>
dummy
<button onclick="dummy()">
</body>
</html>

In this case in saved page both iframes will be same as the 'iframe1' before
saving. So if you click the button in just one iframe the saved page will be not
fully accurate.

In this particular case the iframe contents are saved, because finishing the
file wins the race before aborting the write, because of exceptional event of
trying to write to an already closed file. However with some more complex TC it
could be the other way round. So the patch actually fixes it by letting write at
least one file. There is no regression in any case.


> I appreciate your effort on this CL so far. This is indeed an old bug. My
> suggestion was that we figure out what the correct behavior should be rather
> than dismiss it as being too much effort.

Thanks. I'm afraid the real fix might require a big effort - in worst case even
re-writing big part of that feature...

> I'm happy with an interim fix as long
> as one is reliably better than what's there now and doesn't negatively affect
> code health.

I think it is. Can't be 100% though. It's like ~98%.


On 2014/04/15 20:47:52, rdsmith wrote:
> My perspective on this type of change is that user experience comes first,
with
> code health a close second, and that with a project like Chromium, it's
> depressingly easy to regress on both those metrics if we aren't careful.
> 
> If we were sure that this would improve the user experience for the majority
of
> users, I'd be in favor of doing some variant of it even though it's a
workaround
> rather than a real fix (though I'd want to clean it up some; I have a personal
> hobbyhorse around adding booleans to a class to represent variants on core
> state).  But I don't think we understand the universe of possible failure
modes
> well enough to understand what overall effect this would have.  Maybe in those
> cases when a file is written to twice, the second write is more often the
> correct one?  Maybe the majority of cases have interleaved writes that leave
the
> file completely garbaged, and stopping after the first one finishes won't
help? 
> 
> 
> Normally I'd say something like "let's add some UMA and gather that data!" 
But
> I suspect in this case that would be as much if not more work as actually
fixing
> the problem :-{.   So, with apologies for time wasted, if you don't have time
to
> do a complete fix, I'd recommend dropping this CL.  At least the work helped
you
> learn the code better? :-J

No need for apologies. The patch is already applied in our repo. So even if you
finally reject this patch, the time is not wasted by any means.

If I deduce correctly, the patch will be accepted if I prove it does have
positive impact on user experience (fixes some real cases) and doesn't regress
anything. So now I have three choices.

1) Drop the patch.
2) Prove that it does have positive impact and doesn't regress anything.
Actually I'm about 98% sure about it. Our QAs also have done some work to
confirm it or at least reduce the risk.
3) Provide the fully proper fix.

I'll see what I do - let's keep this open for at least a while then. Dropping
the patch is probably the last thing I would like to happen. So that leaves me
with 2) or 3).

[Jay Civelli, 2016-10-21 22:43:21]

Can this CL be closed?