Test for form submission targeting remote frame.

Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.

Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.  In particular, the first notifyDone()
call in the new form-targets-cross-site-frame.html test would not finish
the test, because of (1).

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.

BUG=585284
Committed: https://crrev.com/af6f5247c144b187499b231d13e326add0963089
Cr-Commit-Position: refs/heads/master@{#396920}

[Łukasz Anforowicz, 2016-05-19 23:01:15]

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

[Łukasz Anforowicz, 2016-05-19 23:01:16]

Dirk, could you please take a look at the change I am making in
components/test_runner/test_runner.cc?  This is fixing a problem my earlier CL
introduced (as I tried to explain in a comment below).

If this looks good, then I'll ask Alex later to review the new layout test.

https://codereview.chromium.org/1989103007/diff/1/components/test_runner/test...
File components/test_runner/test_runner.cc (left):

https://codereview.chromium.org/1989103007/diff/1/components/test_runner/test...
components/test_runner/test_runner.cc:1932: weak_factory_.InvalidateWeakPtrs();
If I don't remove this, then calling testRunner.notifyDone() will invalidate
TestRunner's weak pointers and therefore will invalidate all TestRunner-specific
bindings held by TestRunnerBindings.  This undesirable behavior of
testRunner.notifyDone was introduced in
https://codereview.chromium.org/1852603002 which got rid of
TestRunner::task_list_ and reused TestRunner::weak_factory_ for cancellation of
callbacks.

I think it is fine to remove this call, because everything will still be
cleaned-up before the next test, because we call
weak_factory_.InvalidateWeakPtrs() from TestRunner::Reset.

One could argue that we might want to still cancel pending callbacks, to avoid
any activity while BlinkTestRunner is working on creating the final test dump
(text, pixel, etc.).  To accomplish this, we might move the
weak_factory_.InvalidateWeakPtrs() call into *Complete*NotifyDone method, right
before it calls delegate_->TestFinished().  I think this would be a wrong thing
to do because it wouldn't really help: it wouldn't stop *future* callbacks (i.e.
using weak pointers obtained in the future, before the next call to
TestRunner::Reset).  Since doing this wouldn't really help, let's just not add
this extra complexity.

I do note that all the tests pass without the removed code.

Sounds good?

[Łukasz Anforowicz, 2016-05-19 23:12:55]

<adding jochen@ and mkwst@ to CC, in case they want to take a look later [they
are both out of office right now]>

[Dirk Pranke, 2016-05-20 01:13:29]

dpranke@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[Dirk Pranke, 2016-05-20 01:13:29]

I'm out of my depth on this one. What you wrote sounds plausible, but it's
probably better for esprehn@ or haraken@ to review it.

[haraken, 2016-05-20 01:52:04]

On 2016/05/20 01:13:29, Dirk Pranke wrote:
> I'm out of my depth on this one. What you wrote sounds plausible, but it's
> probably better for esprehn@ or haraken@ to review it.

(I'm more out of my depth on this one... :/)

[dcheng, 2016-05-20 07:08:16]

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

[dcheng, 2016-05-20 07:08:17]

Can you update the CL description with why NotifyDone() shouldn't cancel pending
tasks? i.e. how was this behavior breaking the attached test (I assume)?

[Łukasz Anforowicz, 2016-05-20 16:23:59]

Description was changed from

==========
Test for form submission targeting remote frame.

BUG=585284
==========

to

==========
Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.


Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.


BUG=585284
==========

[Łukasz Anforowicz, 2016-05-20 16:58:42]

Description was changed from

==========
Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.


Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.


BUG=585284
==========

to

==========
Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.


Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.  In particular, the first notifyDone()
call in the new form-targets-cross-site-frame.html test would not finish
the test, because of (1).

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.


BUG=585284
==========

[Łukasz Anforowicz, 2016-05-20 17:01:32]

Hmmm... I guess I'll just wait until Jochen comes back :-)  (unless the expanded
CL description helps and/or somebody else wants to jump in).

On 2016/05/20 07:08:17, dcheng wrote:
> Can you update the CL description with why NotifyDone() shouldn't cancel
pending
> tasks? i.e. how was this behavior breaking the attached test (I assume)?

Good point.  I've updated the CL description with a more thorough version of
what I had in the CR comments.

I've also added a short test description (in CL description and in the test
body).

And I've also made a few small tweaks here and there:
- Changed subframe's name from "foo" to "cross-site-frame"
- Merged NotifyDone and CompleteNotifyDone methods (since after the first
patchset NotifyDone was simply forwarding to CompleteNotifyDone).

[Łukasz Anforowicz, 2016-05-20 17:03:11]

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

[Łukasz Anforowicz, 2016-05-20 17:03:11]

Alex, can you please take a look at the new test?

[dcheng, 2016-05-20 17:13:11]

Thanks! Based on your updated description, can we just use a different
WeakPtrFactory for the connection between TestRunner and TestRunnerBindings?

[Łukasz Anforowicz, 2016-05-20 17:30:32]

On 2016/05/20 17:13:11, dcheng wrote:
> Thanks! Based on your updated description, can we just use a different
> WeakPtrFactory for the connection between TestRunner and TestRunnerBindings?

We could do this, but I think removing this is the correct thing to do, because:

  - the invalidation doesn't prevent new callbacks (so one can't argue that it
helps "silence" test activity afterwards)

  - there is no timeout timer in TestRunner AFAICT (timeout timer is/was
mentioned in the comment above the invalidation).  Even if there was a timeout
timer, cancelling it in NotifyDone would be wrong - the test is not finished at
this point yet (as pointed out in CL description)

  - tests pass without the invalidation
  
OTOH, I have trouble tracing down why/when the invalidation was introduced,
because I have trouble tracing origins of TestRunner.cpp before
https://codereview.chromium.org/110533009 (I can't find it in Blink/WebKit repo
at that time).

[dcheng, 2016-05-20 17:36:55]

On 2016/05/20 at 17:30:32, lukasza wrote:
> On 2016/05/20 17:13:11, dcheng wrote:
> > Thanks! Based on your updated description, can we just use a different
> > WeakPtrFactory for the connection between TestRunner and TestRunnerBindings?
> 
> We could do this, but I think removing this is the correct thing to do,
because:
> 
>   - the invalidation doesn't prevent new callbacks (so one can't argue that it
helps "silence" test activity afterwards)
> 

Theoretically fixable, though I don't think it's worth it.

>   - there is no timeout timer in TestRunner AFAICT (timeout timer is/was
mentioned in the comment above the invalidation).  Even if there was a timeout
timer, cancelling it in NotifyDone would be wrong - the test is not finished at
this point yet (as pointed out in CL description)

It kind of is though: I'd argue that calling notifyDone() before waitUntilDone()
is a race.

> 
>   - tests pass without the invalidation

Yeah, but since no one is clear why it was introduced, it makes it harder to
LGTM. It's the more conservative change and one that reviewers that aren't
familiar with the code are more likely to accept =)

>   
> OTOH, I have trouble tracing down why/when the invalidation was introduced,
because I have trouble tracing origins of TestRunner.cpp before
https://codereview.chromium.org/110533009 (I can't find it in Blink/WebKit repo
at that time).

[Łukasz Anforowicz, 2016-05-20 18:03:14]

On 2016/05/20 17:36:55, dcheng wrote:
> On 2016/05/20 at 17:30:32, lukasza wrote:
> > On 2016/05/20 17:13:11, dcheng wrote:
> > > Thanks! Based on your updated description, can we just use a different
> > > WeakPtrFactory for the connection between TestRunner and
TestRunnerBindings?
> > 
> > We could do this, but I think removing this is the correct thing to do,
> because:
> > 
> >   - the invalidation doesn't prevent new callbacks (so one can't argue that
it
> helps "silence" test activity afterwards)
> > 
> 
> Theoretically fixable, though I don't think it's worth it.
> 
> >   - there is no timeout timer in TestRunner AFAICT (timeout timer is/was
> mentioned in the comment above the invalidation).  Even if there was a timeout
> timer, cancelling it in NotifyDone would be wrong - the test is not finished
at
> this point yet (as pointed out in CL description)
> 
> It kind of is though: I'd argue that calling notifyDone() before
waitUntilDone()
> is a race.

It is not a race in the new test.

Besides, not calling waitUntilDone is only one scenario that would prevent
notifyDone from finishing a test.  Unfinished load is another one (i.e. if
TestRunner::top_loading_frame_ is not null) and pending items in
TestRunner::WorkQueue are another (typically used during back/forward navigation
tests).
> 
> > 
> >   - tests pass without the invalidation
> 
> Yeah, but since no one is clear why it was introduced, it makes it harder to
> LGTM. It's the more conservative change and one that reviewers that aren't
> familiar with the code are more likely to accept =)
>
I know :-/.  OTOH, I am not in much hurry with this CL, so it can wait for a
while (hopefully jochen@ will be back in a week or two :-).
> >   
> > OTOH, I have trouble tracing down why/when the invalidation was introduced,
> because I have trouble tracing origins of TestRunner.cpp before
> https://codereview.chromium.org/110533009 (I can't find it in Blink/WebKit
repo
> at that time).

[alexmos, 2016-05-25 17:59:43]

While we're waiting for the rest of this to get reviewed, I took a look at the
new test, and that LGTM with nit.

https://codereview.chromium.org/1989103007/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame.html
(right):

https://codereview.chromium.org/1989103007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame.html:22:
testRunner.logToStderr("Caling waitUntilDone");
nit: s/Caling/Calling/, but are the stderr logs even necessary here and below?

[Łukasz Anforowicz, 2016-05-25 21:31:43]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org
- dpranke@chromium.org, esprehn@chromium.org, haraken@chromium.org

[Łukasz Anforowicz, 2016-05-25 21:31:45]

Jochen, could you please take a look when you are back?  (no rush, this can wait
for a week or two).  Note that some discussion about //components/test_runner
changes took place in https://codereview.chromium.org/1989103007/#msg11 and
https://codereview.chromium.org/1989103007/#msg15.

https://codereview.chromium.org/1989103007/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame.html
(right):

https://codereview.chromium.org/1989103007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/form-targets-cross-site-frame.html:22:
testRunner.logToStderr("Caling waitUntilDone");
On 2016/05/25 17:59:43, alexmos wrote:
> nit: s/Caling/Calling/, but are the stderr logs even necessary here and below?

I've removed the logging statements.

[jochen (gone - plz use gerrit), 2016-05-30 13:52:16]

there used to be a timeout timer, but it was flaky. Instead, the python harness
is now doing the timeouting

so lgtm

[Łukasz Anforowicz, 2016-05-31 15:50:00]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-05-31 15:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1989103007/#ps40001
(title: "Removing unnecessary logging statements.")

[commit-bot: I haz the power, 2016-05-31 15:50:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103007/40001

[commit-bot: I haz the power, 2016-05-31 20:38:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-31 20:45:30]

Description was changed from

==========
Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.


Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.  In particular, the first notifyDone()
call in the new form-targets-cross-site-frame.html test would not finish
the test, because of (1).

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.


BUG=585284
==========

to

==========
Test for form submission targeting remote frame.

Description of the new form-targets-cross-site-frame.html test.
===============================================================

The test verifies that a form with target="crossSiteFrame" works fine
(especially in presence of out-of-process-iframes, aka OOPIFs, aka
blink::RemoteFrames which are present in --site-per-process mode).

The test finishes successfully, if form-target.pl ends up calling
testRunner.notifyDone() after main frame calls testRuner.waitUntilDone
and submits the form.  Expected test output helps verify that the
correct HTTP method was used and that test field's value from the form
was propagated correctly.

Required test harness fix (under components/test_runner)
========================================================

Context
-------

Before https://crrev.com/1852603002 (which I landed about a month ago):

- Cancellation of TestRunner's callbacks was using WebTaskList, instead
  of reusing base::WeakPtr to cancel callbacks.

- TestRunner::NotifyDone would cancel pending callbacks, before completing
  notifyDone processing.

After https://crrev.com/1852603002:

- WebTaskList was removed and replaced with using base::WeakPtr (yay!)

- TestRunner::NotifyDone was modified to call
  weak_factory_.InvalidateWeakPtrs()
  instead of task_list_.RevokeAll().
  This has led to the problem below.

Problem
-------

Calling weak_factory_.InvalidateWeakPtrs() from TestRunner::NotifyDone
would cancel pending callbacks (as before), but invalidated weak
pointers are also used for other things - i.e. as a link between
TestRunnerBindings and TestRunner instance.  This meant that
https://crrev.com/1852603002 accidentally changed the semantics of
testRunner.notifyDone().

Before https://crrev.com/1852603002 TestRunnerBindings would continue to
work after a call to testRunner.notifyDone().  Keeping these bindings
working is important, because testRunner.notifyDone() might *not* finish
the test when 1) testRunner.waitUntilDone() was not called yet, or
2) there are pending work items in TestRunner::WorkQueue or 3) we are
still tracking and waiting to finish loading of
TestRunner::top_loading_frame_.  In particular, the first notifyDone()
call in the new form-targets-cross-site-frame.html test would not finish
the test, because of (1).

Fix
---

It turns out that we can simply remove the call to InvalidateWeakPtrs
from TestRunner::NotifyDone.  This is okay because:

- InvalidateWeakPtrs will still be called in TestRunner::Reset (which is
  called between tests), so we still will insulate one test from another.

- Calling InvalidateWeakPtrs from TestRunner::NotifyDone cancels *pending*
  callbacks, but new callbacks created between TestRunner::NotifyDone and
  TestRunner::Reset would still be allowed to run.  Therefore the call to
  InvalidateWeakPtrs doesn't robustly accomplish its original purpose.

Since after the fix TestRunner::NotifyDone simply forwards the call
to TestRunner::CompleteNotifyDone, these 2 methods can be merged
together.

BUG=585284

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

[commit-bot: I haz the power, 2016-05-31 20:45:31]

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