Fix InternalsGeolocation::setGeolocationClientMock to actually set the test client for all Geolocat…

Fix InternalsGeolocation::setGeolocationClientMock to actually set the test client for all GeolocationController.

This was caught by kenrb. It didn't manifest itself because it's a test-only method, and none of the geolocation tests which use this do so for an iframe.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175907

[jam, 2014-06-09 18:16:28]

Michael: this is fixing an error from my previous cl which Ken caught.

One interesting thing is that
fast/dom/Geolocation/callback-to-deleted-context.html is failing on one bot. It
turns out that it's also failing without this cl. It fails depending on the
order that it's run. If it's run alone, it fails.

I don't understand why we even want this behavior, which is that the geolocation
callback is called even if the frame that it's in goes away. It does appear that
I only changed the test-behavior anyways, and not the production code. The
original bug that added this was https://bugs.webkit.org/show_bug.cgi?id=40162,
and looking at the first patch it appears it might have been a security bug in
the non-v8 bindings.

If you agree with the above, I can remove that test?

[Michael van Ouwerkerk, 2014-06-10 13:58:43]

On 2014/06/09 18:16:28, jam wrote:
> Michael: this is fixing an error from my previous cl which Ken caught.
> 
> One interesting thing is that
> fast/dom/Geolocation/callback-to-deleted-context.html is failing on one bot.
It
> turns out that it's also failing without this cl. It fails depending on the
> order that it's run. If it's run alone, it fails.
> 
> I don't understand why we even want this behavior, which is that the
geolocation
> callback is called even if the frame that it's in goes away. It does appear
that
> I only changed the test-behavior anyways, and not the production code. The
> original bug that added this was
https://bugs.webkit.org/show_bug.cgi?id=40162,
> and looking at the first patch it appears it might have been a security bug in
> the non-v8 bindings.
> 
> If you agree with the above, I can remove that test?

Your change lgtm.

I think callback-to-deleted-context.html tests something that should work,
though the particular use case is contrived. What it does is call
getCurrentPosition in the parent frame, passing in function literals from the
child frame. I'd expect those literals to not be garbage collected until all
references to it are gone, and the parent is still there right?

[jam, 2014-06-10 16:22:48]

On 2014/06/10 13:58:43, Michael van Ouwerkerk wrote:
> On 2014/06/09 18:16:28, jam wrote:
> > Michael: this is fixing an error from my previous cl which Ken caught.
> > 
> > One interesting thing is that
> > fast/dom/Geolocation/callback-to-deleted-context.html is failing on one bot.
> It
> > turns out that it's also failing without this cl. It fails depending on the
> > order that it's run. If it's run alone, it fails.
> > 
> > I don't understand why we even want this behavior, which is that the
> geolocation
> > callback is called even if the frame that it's in goes away. It does appear
> that
> > I only changed the test-behavior anyways, and not the production code. The
> > original bug that added this was
> https://bugs.webkit.org/show_bug.cgi?id=40162,
> > and looking at the first patch it appears it might have been a security bug
in
> > the non-v8 bindings.
> > 
> > If you agree with the above, I can remove that test?
> 
> Your change lgtm.
> 
> I think callback-to-deleted-context.html tests something that should work,
> though the particular use case is contrived. What it does is call
> getCurrentPosition in the parent frame, passing in function literals from the
> child frame. I'd expect those literals to not be garbage collected until all
> references to it are gone, and the parent is still there right?

Thanks, I misread the test. I looked into this more, and it was because the test
controller wasn't set for the iframe. It was caused by incorrect travesal of all
frames. I fixed it now in patchset 2.

[jam, 2014-06-10 16:22:50]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-06-10 16:23:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jam@chromium.org/322583002/20001

[commit-bot: I haz the power, 2014-06-10 17:29:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10980)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11452)

[commit-bot: I haz the power, 2014-06-10 18:06:29]

Change committed as 175907