Do not SendPasswordForms when LocalFrame is being detached

components/autofill/content/renderer/password_autofill_agent.cc

Index: components/autofill/content/renderer/password_autofill_agent.cc
diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
index 0397c3b7ffd4a80fb39fcdb8b8133cfcfd24b1ff..521fd48258658bca126c1dc06153f356b19ffbbb 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -946,7 +946,10 @@ void PasswordAutofillAgent::SendPasswordForms(bool only_visible) {
     logger->LogBoolean(Logger::STRING_ONLY_VISIBLE, only_visible);
   }
 
-  blink::WebFrame* frame = render_frame()->GetWebFrame();
+  blink::WebLocalFrame* frame = render_frame()->GetWebFrame();

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

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 07:03:01]

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?

[vabr (Chromium), 2016/10/07 11:00:48]

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?

[vabr (Chromium), 2016/10/07 11:00:48]

+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]

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]

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.

+  if (frame->isInStopAllLoaders())
+    return;
+
   // Make sure that this security origin is allowed to use password manager.
   blink::WebSecurityOrigin origin = frame->document().getSecurityOrigin();
   if (logger) {