Do not SendPasswordForms when LocalFrame is being detached

Do not SendPasswordForms when LocalFrame is being detached

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654
Committed: https://crrev.com/a398abc5085c5dc2d8db3076543072d1f196d668
Cr-Commit-Position: refs/heads/master@{#424692}

[kojii, 2016-10-07 06:18:38]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 06:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-07 06:39:41]

Description was changed from

==========
WIP: autofill-closing

BUG=
==========

to

==========
Do not SendPasswordForms when inStopAllLoaders

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078
==========

[esprehn, 2016-10-07 06:55:23]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-10-07 06:55:23]

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
Why do we get here when doing stopAllLoaders? I'd much rather disable the
clients than plumb more state out through the web layer.

[kojii, 2016-10-07 06:59:11]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org, vabr@chromium.org

[kojii, 2016-10-07 07:03:01]

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
On 2016/10/07 at 06:55:23, esprehn wrote:
> Why do we get here when doing stopAllLoaders? I'd much rather disable the
clients than plumb more state out through the web layer.

We seem to call didFinishLoad() even when close/cancel, see stack at
https://crbug.com/561873#c127.

We thought about not calling didFinishLoad() on this case, but it changes
behavior for all observers and were scared to do so. Do you think we should go
that way, or see any better place to stop in the stack?

[commit-bot: I haz the power, 2016-10-07 07:58:55]

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

[commit-bot: I haz the power, 2016-10-07 07:58:55]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-10-07 11:00:49]

In general LGTM, because this will improve the current situation. If we can fix
this completely inside Blink, as esprehn@ said, that would be even better,
though.

Cheers,
Vaclav

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
Just to make sure I understand correctly -- it is OK to assume that frame is
WebLocalFrame (as opposed to WebRemoteFrame), because it comes from the
RenderFrame living in the same renderer process?

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
On 2016/10/07 07:03:01, kojii wrote:
> On 2016/10/07 at 06:55:23, esprehn wrote:
> > Why do we get here when doing stopAllLoaders? I'd much rather disable the
> clients than plumb more state out through the web layer.
> 
> We seem to call didFinishLoad() even when close/cancel, see stack at
> https://crbug.com/561873#c127.
> 
> We thought about not calling didFinishLoad() on this case, but it changes
> behavior for all observers and were scared to do so. Do you think we should go
> that way, or see any better place to stop in the stack?

+1 to esprehn's concern. The comment at WebFrameClient::didFinishLoad says: "The
frame's document and all of its subresources succeeded to load," while
FrameLoader::stopAllLoaders warns that "All callers need to either protect the
LocalFrame or guarantee they won't in any way access the LocalFrame". That
sounds like a surprising request after the frame is told to have loaded
successfully.

Are there many observers which are relying on seeing didFinishLoad even if
stopAllLoaders was called? Could we check them by passing nullptr for the frame
argument if coming from stpAllLoaders and see if they try to dereference it?

[kojii, 2016-10-07 11:32:44]

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
On 2016/10/07 at 11:00:48, vabr (Chromium) wrote:
> Just to make sure I understand correctly -- it is OK to assume that frame is
WebLocalFrame (as opposed to WebRemoteFrame), because it comes from the
RenderFrame living in the same renderer process?

I'm not very familiar with code outside Blink, but in this case the function
returns WebLocalFrame, so I assume it's safe.
https://cs.chromium.org/chromium/src/content/public/renderer/render_frame.h?q...

[kojii, 2016-10-07 11:45:08]

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:949:
blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
On 2016/10/07 at 11:00:48, vabr (Chromium) wrote:
> On 2016/10/07 07:03:01, kojii wrote:
> > On 2016/10/07 at 06:55:23, esprehn wrote:
> > > Why do we get here when doing stopAllLoaders? I'd much rather disable the
> > clients than plumb more state out through the web layer.
> > 
> > We seem to call didFinishLoad() even when close/cancel, see stack at
> > https://crbug.com/561873#c127.
> > 
> > We thought about not calling didFinishLoad() on this case, but it changes
> > behavior for all observers and were scared to do so. Do you think we should
go
> > that way, or see any better place to stop in the stack?
> 
> +1 to esprehn's concern. The comment at WebFrameClient::didFinishLoad says:
"The frame's document and all of its subresources succeeded to load," while
FrameLoader::stopAllLoaders warns that "All callers need to either protect the
LocalFrame or guarantee they won't in any way access the LocalFrame". That
sounds like a surprising request after the frame is told to have loaded
successfully.
> 
> Are there many observers which are relying on seeing didFinishLoad even if
stopAllLoaders was called? Could we check them by passing nullptr for the frame
argument if coming from stpAllLoaders and see if they try to dereference it?

Actually both tkent@ and I +1 too; cancel and close firing didFinishLoad() looks
like an error. On the other hand, there are 53 subclasses, and 18 out of that
override didFinishLoad(), so we were a bit scary to change the behavior of
didFinishLoad(). Unlike what the comment says, the current behavior looks more
like "finally" than "succeeded", or maybe "partial success" was put into
"success" bucket.

I'll look around more code, and also hope if esprehn@ can give some advices for
how to implement this.

Thank you for having a quick look BTW, and for your agreement on general
direction.

[vabr (Chromium), 2016-10-07 12:05:42]

On 2016/10/07 11:45:08, kojii wrote:
>
https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/2398733004/diff/1/components/autofill/content...
> components/autofill/content/renderer/password_autofill_agent.cc:949:
> blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
> On 2016/10/07 at 11:00:48, vabr (Chromium) wrote:
> > On 2016/10/07 07:03:01, kojii wrote:
> > > On 2016/10/07 at 06:55:23, esprehn wrote:
> > > > Why do we get here when doing stopAllLoaders? I'd much rather disable
the
> > > clients than plumb more state out through the web layer.
> > > 
> > > We seem to call didFinishLoad() even when close/cancel, see stack at
> > > https://crbug.com/561873#c127.
> > > 
> > > We thought about not calling didFinishLoad() on this case, but it changes
> > > behavior for all observers and were scared to do so. Do you think we
should
> go
> > > that way, or see any better place to stop in the stack?
> > 
> > +1 to esprehn's concern. The comment at WebFrameClient::didFinishLoad says:
> "The frame's document and all of its subresources succeeded to load," while
> FrameLoader::stopAllLoaders warns that "All callers need to either protect the
> LocalFrame or guarantee they won't in any way access the LocalFrame". That
> sounds like a surprising request after the frame is told to have loaded
> successfully.
> > 
> > Are there many observers which are relying on seeing didFinishLoad even if
> stopAllLoaders was called? Could we check them by passing nullptr for the
frame
> argument if coming from stpAllLoaders and see if they try to dereference it?
> 
> Actually both tkent@ and I +1 too; cancel and close firing didFinishLoad()
looks
> like an error. On the other hand, there are 53 subclasses, and 18 out of that
> override didFinishLoad(), so we were a bit scary to change the behavior of
> didFinishLoad(). Unlike what the comment says, the current behavior looks more
> like "finally" than "succeeded", or maybe "partial success" was put into
> "success" bucket.
> 
> I'll look around more code, and also hope if esprehn@ can give some advices
for
> how to implement this.
> 
> Thank you for having a quick look BTW, and for your agreement on general
> direction.

Thanks for the quick reply. Your considerations and plans SGTM!

Cheers,
Vaclav

[esprehn, 2016-10-08 03:06:53]

Okay, can we get a tracking bug field to fix this more generally and a TODO in
the code? :)

Also this needs a test. :D

[dcheng, 2016-10-08 05:44:59]

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

[dcheng, 2016-10-08 05:45:00]

So I'm the person who added this comment to stopAllLoaders():
  // Warning: stopAllLoaders can and will detach the LocalFrame out from under
  // you. All callers need to either protect the LocalFrame or guarantee they
  // won't in any way access the LocalFrame after stopAllLoaders returns.

I wrote this comment before we implemented Oilpan. Since stopAllLoaders() can
run script, it is important to protect the reference to prevent |this| from
being destroyed.

However, this comment is obsolete now: with Oilpan, |this| will either be on the
stack or in registers, so it will not be marked and swept in the middle of
detaching. However, we still must be very careful: that's why there are a lot of
checks like if (!client()). See the comments in
https://codereview.chromium.org/2395423002/ for the full details.

In the bug, it was noted that we're crashing with the following stack:
  RenderViewImpl::Close()
  RenderWidget::Close()
  WebViewImpl::close()
  Page::willBeDestroyed()
  LocalFrame::detach()
  FrameLoader::stopAllLoders()
  ResourceFetcher::stopFetching()
  ResourceLoader::cancel()
  ResourceLoader::didFail()
  FrameFetchContext::didLoadResource()
  FrameLoader::checkCompleted()
  FrameLoaderClientImpl::dispatchDidFinishLoad()
  RenderFrameImpl::didFinishLoad()
  PasswordAutofillAgent::SendPasswordForms()
  form_util::AreFormContentsVisisble()
  form_util::IsSomeControlElementVisisble()
  form_util::IsWebNodeVisible()
  WebElement::hasNonEmptyLayoutSize()
  Document::updateStyleAndLayoutIgnoringPendingStylesheets()

Do you know which call to stopAllLoaders() it is crashing at? There are two
calls in LocalFrame::detach(). I am worried that even if we prevent password
autofill from doing this work, other code can trigger layout during
LocalFrame::detach: I think script can trivially do this during unload events,
for example.

[kojii, 2016-10-09 04:22:58]

On 2016/10/08 at 05:45:00, dcheng wrote:
> 
> Do you know which call to stopAllLoaders() it is crashing at? There are two
calls in LocalFrame::detach(). I am worried that even if we prevent password
autofill from doing this work, other code can trigger layout during
LocalFrame::detach: I think script can trivially do this during unload events,
for example.

The first one, see https://crash.corp.google.com/browse?stbtiq=025ab34300000000
(sorry, Googlers only)

Another idea is to add "isLoadingCanceled" or "isFrameDetaching", rather than
"inStopAllLoaders", which is more general and less impl dependent. Still
requires observers to plumb, but slightly better abstracted?

[dcheng, 2016-10-10 03:52:16]

On 2016/10/09 04:22:58, kojii wrote:
> On 2016/10/08 at 05:45:00, dcheng wrote:
> > 
> > Do you know which call to stopAllLoaders() it is crashing at? There are two
> calls in LocalFrame::detach(). I am worried that even if we prevent password
> autofill from doing this work, other code can trigger layout during
> LocalFrame::detach: I think script can trivially do this during unload events,
> for example.
> 
> The first one, see
https://crash.corp.google.com/browse?stbtiq=025ab34300000000
> (sorry, Googlers only)
> 
> Another idea is to add "isLoadingCanceled" or "isFrameDetaching", rather than
> "inStopAllLoaders", which is more general and less impl dependent. Still
> requires observers to plumb, but slightly better abstracted?

Thanks for the crash link! Given that, my concern is if we have a page like
this:

<script>window.onunload = function() { document.body.offsetTop; }</script>

This will trigger layout inside LocalFrame::detach and potentially trigger the
same crash. At the point when we can still run script in LocalFrame::detach, the
entire stack of WebLocalFrame/LocalFrame/FrameView/LocalDOMWindow/Document
should still be internally consistent, so the fact that it isn't is somewhat
surprising.

[kojii, 2016-10-11 03:36:11]

Description was changed from

==========
Do not SendPasswordForms when inStopAllLoaders

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078
==========

to

==========
Do not SendPasswordForms when inStopAllLoaders

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654
==========

[kojii, 2016-10-11 03:57:50]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 03:57:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-11 04:06:39]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 04:06:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-11 04:06:59]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-11 05:16:44]

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

[commit-bot: I haz the power, 2016-10-11 05:16:45]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-11 05:24:22]

A few changes from PS1:

* Tracking bug filed http://crbug.com/654654
* Added comments
* Changed from isInStopAllLoaders -> isDetached to be more abstracted

As in the bug description, I acknowledge this is not addressing the root cause.
Fonts people are struggling to find the root cause of the crashing bug but we
were not succeeded yet, while the crash rate keeps quite high. This is only to
lower the crash rate.

I'm not very familiar with code flows around this, if someone can take over the
real fix, that'd be greatly appreciated.

And please consider this CL as an interim, not necessary if the real fix is
possible in short time frame.

[kojii, 2016-10-11 09:09:11]

it looks like didFinishLoad() should fire on cancel/closing, and it is
observer's responsibility to plumb how it's finished, IIUC the dcheng@'s comment
in http://crbug.com/654654.

If it's a consensus, this patch does the right thing. Though it does not address
the root cause of the crash, not filling passwords on close/cancel looks right
to me.

More thoughts on this? Should I remove the comments to the crbug?

[vabr (Chromium), 2016-10-11 09:36:04]

LGTM, thanks, for the care put into this CL and the associated discussion.

I think I share the understanding of dcheng's comments with kojii@. In
particular, I understand that didFinishLoad is meant to cover also the
cancelling use-case, and that the related comment in the code might be the one
needing fixing.

The current CL is definitely an improvement for password manager, IMO.

Cheers,
Vaclav

[esprehn, 2016-10-11 22:45:52]

https://codereview.chromium.org/2398733004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2398733004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2029: if (!frame() ||
!frame()->client())
I'd prefer if the ->client() check was in core Frame's isDetached(). The way you
have this written isDetached() inside core means something different than
isDetached() inside web/

[dcheng, 2016-10-11 22:47:56]

How will we prevent more places from depending on isDetaching()? I'm worried
that by the time we figure out what's actually causing this bug, we won't be
able to remove this easily.

[esprehn, 2016-10-11 22:55:31]

On 2016/10/11 at 22:47:56, dcheng wrote:
> How will we prevent more places from depending on isDetaching()? I'm worried
that by the time we figure out what's actually causing this bug, we won't be
able to remove this easily.

If I understand that linked bug correctly we can't remove this, since we expect
the embedder to know when to abort even though we're still running the
callbacks?

[dcheng, 2016-10-11 23:09:04]

On 2016/10/11 22:55:31, esprehn wrote:
> On 2016/10/11 at 22:47:56, dcheng wrote:
> > How will we prevent more places from depending on isDetaching()? I'm worried
> that by the time we figure out what's actually causing this bug, we won't be
> able to remove this easily.
> 
> If I understand that linked bug correctly we can't remove this, since we
expect
> the embedder to know when to abort even though we're still running the
> callbacks?

My understanding is that this doesn't solve the actual problem (there's some
issue with font code if layout happens during detach: this can happen without
the password autofill code). However, password autofill is causing so many
crashes that it's worth it to land a stopgap to at least reduce the number.

This is the proposed stopgap, which is fine: however, I'd like to avoid a
situation where more things start branching based on isDetached(): tearing down
frames is already complicated as it is, and if random code starts making
decisions based on this, it's just going to get even harder to figure out what
actually happens during frame detach.

From https://crbug.com/654654, it just seems like no one really knows what the
purpose of these callbacks is anymore. It's entirely possibly that we just
shouldn't check for load completion in a frame that's detaching (which seems
like a perfectly reasonable thing to do). After all, it's silly to trigger a
bunch of work in the embedder when we're about to tear down everything anyway.


One suggestion that might simplify the plumbing is to add a scoper like
SubframeLoadingDisabler: frames that are detaching can be inserted into this
set, and we can have a small helper in public/web that autofill can use to query
this set. Then we don't need to expose it on LocalFrame/WebLocalFrame directly.

[kojii, 2016-10-11 23:56:30]

On 2016/10/11 at 23:09:04, dcheng wrote:
> On 2016/10/11 22:55:31, esprehn wrote:
> > On 2016/10/11 at 22:47:56, dcheng wrote:
> > > How will we prevent more places from depending on isDetaching()? I'm
worried
> > that by the time we figure out what's actually causing this bug, we won't be
> > able to remove this easily.
> > 
> > If I understand that linked bug correctly we can't remove this, since we
expect
> > the embedder to know when to abort even though we're still running the
> > callbacks?
> 
> My understanding is that this doesn't solve the actual problem (there's some
issue with font code if layout happens during detach: this can happen without
the password autofill code). However, password autofill is causing so many
crashes that it's worth it to land a stopgap to at least reduce the number.
> 
> This is the proposed stopgap, which is fine: however, I'd like to avoid a
situation where more things start branching based on isDetached(): tearing down
frames is already complicated as it is, and if random code starts making
decisions based on this, it's just going to get even harder to figure out what
actually happens during frame detach.
> 
> From https://crbug.com/654654, it just seems like no one really knows what the
purpose of these callbacks is anymore. It's entirely possibly that we just
shouldn't check for load completion in a frame that's detaching (which seems
like a perfectly reasonable thing to do). After all, it's silly to trigger a
bunch of work in the embedder when we're about to tear down everything anyway.

You're right on the crashes, but then since you said we should fire
DidFinishLoad on cancel/close, I suspect there are other cases where observers
want to distinguish cancel/close from normal successful completions. From that
perspective, I don't think we can remove this. Wasn't that what you suggested in
the comment?

If people agree to stop firing DidFinishLoad on cancel/close, and things work
without it, this can be removed. But it's probably beyond my expertise to do
this, I hope someone can take that over.

If we keep firing DidFinishLoad on cancel/close, but want to remove this, we
need two conditions:
1. The actual cause of the crashes was solved.
2. Other observers don't need this.
My best guestimate at this point is both are unlikely. If this is the same cause
as http://wkb.ug/36864#c36, it's not been found for 6 years.

Another cleaner possibilities are:
1. To stop cancel/close firing DidFinishLoad but add LoadingCanceled event
instead, and make 18 observers to support that. That way, IsDetached is no
longer needed.
2. The observer already has FrameDetached. The problem here is that it fires
*after* DidFinishLoad. If we could adjust these orders, and check if the 18
observers are ok, that might work too.
These look cleaner to me, but I don't think I can do it right. Again, help from
someone more familiar on these is appreciated.

> One suggestion that might simplify the plumbing is to add a scoper like
SubframeLoadingDisabler: frames that are detaching can be inserted into this
set, and we can have a small helper in public/web that autofill can use to query
this set. Then we don't need to expose it on LocalFrame/WebLocalFrame directly.

Hmm...I don't understand this. We still need to expose
WebSubframeLoadingDisabler, no?

[dcheng, 2016-10-12 00:15:36]

On 2016/10/11 23:56:30, kojii wrote:
> On 2016/10/11 at 23:09:04, dcheng wrote:
> > On 2016/10/11 22:55:31, esprehn wrote:
> > > On 2016/10/11 at 22:47:56, dcheng wrote:
> > > > How will we prevent more places from depending on isDetaching()? I'm
> worried
> > > that by the time we figure out what's actually causing this bug, we won't
be
> > > able to remove this easily.
> > > 
> > > If I understand that linked bug correctly we can't remove this, since we
> expect
> > > the embedder to know when to abort even though we're still running the
> > > callbacks?
> > 
> > My understanding is that this doesn't solve the actual problem (there's some
> issue with font code if layout happens during detach: this can happen without
> the password autofill code). However, password autofill is causing so many
> crashes that it's worth it to land a stopgap to at least reduce the number.
> > 
> > This is the proposed stopgap, which is fine: however, I'd like to avoid a
> situation where more things start branching based on isDetached(): tearing
down
> frames is already complicated as it is, and if random code starts making
> decisions based on this, it's just going to get even harder to figure out what
> actually happens during frame detach.
> > 
> > From https://crbug.com/654654, it just seems like no one really knows what
the
> purpose of these callbacks is anymore. It's entirely possibly that we just
> shouldn't check for load completion in a frame that's detaching (which seems
> like a perfectly reasonable thing to do). After all, it's silly to trigger a
> bunch of work in the embedder when we're about to tear down everything anyway.
> 
> You're right on the crashes, but then since you said we should fire
> DidFinishLoad on cancel/close, I suspect there are other cases where observers
> want to distinguish cancel/close from normal successful completions. From that
> perspective, I don't think we can remove this. Wasn't that what you suggested
in
> the comment?

I don't really know what the semantics should be without doing a lot more
investigation. =)

I think it's reasonable for clients to expect this call if we call window.stop
(which also uses stopAllLoaders, IIRC).

However, I think it might also be reasonable for clients to not expect loading
callbacks at all if we're in the middle of LocalFrame::detach(): inside Blink,
we already suppress some things during detach: for example, we don't allow
creation of new subframes. Not telling the embedder that the load is complete
might also be a reasonable thing to do.

> 
> If people agree to stop firing DidFinishLoad on cancel/close, and things work
> without it, this can be removed. But it's probably beyond my expertise to do
> this, I hope someone can take that over.

Yes, I think that's reasonable: I don't expect the loading callbacks to be
resolved in this patch. I'm just trying to make sure we have room to fix this in
the future (i.e. by avoiding additional dependencies on isDetached()).

> 
> If we keep firing DidFinishLoad on cancel/close, but want to remove this, we
> need two conditions:
> 1. The actual cause of the crashes was solved.
> 2. Other observers don't need this.
> My best guestimate at this point is both are unlikely. If this is the same
cause
> as http://wkb.ug/36864#c36, it's not been found for 6 years.
> 
> Another cleaner possibilities are:
> 1. To stop cancel/close firing DidFinishLoad but add LoadingCanceled event
> instead, and make 18 observers to support that. That way, IsDetached is no
> longer needed.
> 2. The observer already has FrameDetached. The problem here is that it fires
> *after* DidFinishLoad. If we could adjust these orders, and check if the 18
> observers are ok, that might work too.
> These look cleaner to me, but I don't think I can do it right. Again, help
from
> someone more familiar on these is appreciated.
> 
> > One suggestion that might simplify the plumbing is to add a scoper like
> SubframeLoadingDisabler: frames that are detaching can be inserted into this
> set, and we can have a small helper in public/web that autofill can use to
query
> this set. Then we don't need to expose it on LocalFrame/WebLocalFrame
directly.
> 
> Hmm...I don't understand this. We still need to expose
> WebSubframeLoadingDisabler, no?

Yeah, but then it wouldn't have to be exposed directly on LocalFrame /
WebLocalFrame. I guess my hope was to try to isolate it to a small interface
outside of WebLocalFrame that is hopefully easy to audit and remove down the
road.

[kojii, 2016-10-12 00:48:34]

2016/10/12 午前9:15 <dcheng@chromium.org>:

>
> > One suggestion that might simplify the plumbing is to add a scoper like
> SubframeLoadingDisabler: frames that are detaching can be inserted into
this
> set, and we can have a small helper in public/web that autofill can use to
query
> this set. Then we don't need to expose it on LocalFrame/WebLocalFrame
directly.
>
> Hmm...I don't understand this. We still need to expose
> WebSubframeLoadingDisabler, no?

Yeah, but then it wouldn't have to be exposed directly on LocalFrame /
WebLocalFrame. I guess my hope was to try to isolate it to a small interface
outside of WebLocalFrame that is hopefully easy to audit and remove down the
road.


Ok, but we can't expose static methods, can we? Should I link to the
current insurance from somewhere? But then we will add such method, is this
worth?

... Maybe I'm still missing something...

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kojii, 2016-10-12 00:48:34]

2016/10/12 午前9:15 <dcheng@chromium.org>:

>
> > One suggestion that might simplify the plumbing is to add a scoper like
> SubframeLoadingDisabler: frames that are detaching can be inserted into
this
> set, and we can have a small helper in public/web that autofill can use to
query
> this set. Then we don't need to expose it on LocalFrame/WebLocalFrame
directly.
>
> Hmm...I don't understand this. We still need to expose
> WebSubframeLoadingDisabler, no?

Yeah, but then it wouldn't have to be exposed directly on LocalFrame /
WebLocalFrame. I guess my hope was to try to isolate it to a small interface
outside of WebLocalFrame that is hopefully easy to audit and remove down the
road.


Ok, but we can't expose static methods, can we? Should I link to the
current insurance from somewhere? But then we will add such method, is this
worth?

... Maybe I'm still missing something...

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-10-12 00:54:11]

That all seems very high overhead, lets just give this a terrible name like
isFrameDetachedForSpecialOneOffStopTheCrashingHackBug561873() (totally serious)

[kojii, 2016-10-12 01:30:57]

On 2016/10/12 at 00:54:11, esprehn wrote:
> That all seems very high overhead, lets just give this a terrible name like
isFrameDetachedForSpecialOneOffStopTheCrashingHackBug561873() (totally serious)

Ok, I'll update the patch.

[dcheng, 2016-10-12 02:53:19]

On 2016/10/12 01:30:57, kojii wrote:
> On 2016/10/12 at 00:54:11, esprehn wrote:
> > That all seems very high overhead, lets just give this a terrible name like
> isFrameDetachedForSpecialOneOffStopTheCrashingHackBug561873() (totally
serious)
> 
> Ok, I'll update the patch.

Thanks, this works for me too.

Once this lands, I'll experiment with suppressing the loading callbacks for a
frame that's detaching. If we're lucky, it won't be too hard... =)

[kojii, 2016-10-12 03:29:12]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 03:29:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-12 05:08:05]

Description was changed from

==========
Do not SendPasswordForms when inStopAllLoaders

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654
==========

to

==========
Do not SendPasswordForms when LocalFrame is being detached

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654
==========

[kojii, 2016-10-12 05:08:32]

PTAL, renamed to isFrameDetachedForSpecialOneOffStopTheCrashingHackBug561873().

[commit-bot: I haz the power, 2016-10-12 05:12:49]

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

[commit-bot: I haz the power, 2016-10-12 05:12:50]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-12 05:15:39]

Blink LGTM, thanks!

[vabr (Chromium), 2016-10-12 07:56:17]

isFrameDetachedForSpecialOneOffStopTheCrashingHackBug561873 LGTM, thank you all
for the very informative discussion and looking into this!

Cheers,
Vaclav

[kojii, 2016-10-12 08:32:11]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-10-12 08:32:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 08:38:24]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-12 08:39:44]

Description was changed from

==========
Do not SendPasswordForms when LocalFrame is being detached

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654
==========

to

==========
Do not SendPasswordForms when LocalFrame is being detached

This patch stops PasswordAutofillAgent::SendPasswordForms() to
enumerates forms in the document when LocalFrame is being detached.

This patch does not address the root cause of issue 561873 nor
issue 595078, but it is expected to reduce the crashes by large, and
it is reasonable not to fill passwords when Page is being destroyed.

BUG=561873, 595078, 654654

Committed: https://crrev.com/a398abc5085c5dc2d8db3076543072d1f196d668
Cr-Commit-Position: refs/heads/master@{#424692}
==========

[commit-bot: I haz the power, 2016-10-12 08:39:46]

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