886026 - Make clearing pending and not-yet-allowed watch requests work

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Josh Matthews [:jdm]]

I suspect we'll assert in debug builds and not cancel the eventual watch in release ones.

Comment 1

[Guilherme Gonçalves [:ggp]]

Just tried a debug build.

You won't actually get an assertion, but since the request is not yet in the list of pending requests (since it hasn't been allowed), it is not cleared. Allowing the request in the prompt causes it to be added to the list of requests as usual; the call to clearWatch is effectively ignored.

Another thing to look out for is clearing requests that are made before the geolocation service has been initialized (as far as I can see, this can only happen if geolocation was disabled via mozsettings). These requests are currently queued (see mPendingRequests) until the geolocation service has been initialized. Do we really need to do that? It seems to make sense to just issue POSITION_UNAVAILABLE until the service is actually running...

Comment 2

[Josh Matthews [:jdm]]

Queuing the pending requests is reasonable; I don't want web authors to have to worry about reissuing requests and hoping for the best.

Comment 3

[Josh Matthews [:jdm]]

I suppose we can store an array of cleared watch IDs and check it whenever Allow is called.

Comment 4

[Guilherme Gonçalves [:ggp]]

Attachment: Test case

A test case for clearing not-yet-allowed requests.

Comment 5

[Guilherme Gonçalves [:ggp]]

(In reply to Josh Matthews [:jdm] from comment #3)
> I suppose we can store an array of cleared watch IDs and check it whenever
> Allow is called.

Since permission requests don't stack, we may want to keep only the active (latest) request, or we risk keeping all old requests alive because of the array.

Comment 6

[Josh Matthews [:jdm]]

Note that I said IDs, not requests.

Comment 7

[Josh Matthews [:jdm]]

By the way, there's a new contributor who appears to be looking into this bug, so please hold off on solving it :)

Comment 8

[Hareesh]

This is my first bug fix ! I have setup my tree and ran mochitest with the test patch above and it fails expected. I am looking for a good starting point to understand how this api works. How does the C++ file I have and the js test file interact ? Can you help this ?

Comment 9

[Josh Matthews [:jdm]]

The JS and C++ code interacts through our webidl and idl interface files - we define interfaces like http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Geolocation.webidl and http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIGeolocationProvider.idl, which are turned into C++ classes with pure virtual methods that are implemented by classes in nsGeolocation.cpp. The same interface definitions give us the ability to call the C++ code from JS and vice versa.

Comment 10

[Hareesh]

Thanks for the info. Mochitest runs fine and it fails for the test case attached with the bug. I have nsGeolocation.cpp setup in Xcode. Is it possible to have a breakpoint in my cpp file and execute geolocation api through browser (using some other test/html file) and step through the execution from there ?

Comment 11

[Josh Matthews [:jdm]]

The answer is likely yes; you need to set up some kind of special target in Xcode to run the Firefox binary in your build directory under mozilla-central. If you use gdb from the terminal window, it's very easy to get working, but there's no Xcode integration.

Comment 12

[Dongie Agnir [:dagnir]]

Hi Josh, was this fixed in mozilla-central?  I applied the patch provided and the test seems to be passing. 

(ran with './mach mochitest-plain dom/tests/mochitest/geolocation/').  Note I had to modify the changes applied in the patch slightly because hg couldn't find dom/tests/mochitest/geolocation/Makefile.in; I added it to mochitest.ini instead in the same directory.

Comment 13

[Josh Matthews [:jdm]]

That behaviour surprises me; I wouldn't expect this to be fixed. Are you running a debug build (https://developer.mozilla.org/en-US/docs/Configuring_Build_Options)?

Comment 14

[Dongie Agnir [:dagnir]]

Ah, no I was not.  I'll rebuild and check again.  Thanks!

Comment 15

[Vinay G Shetty]

Hey jdm, 

Aren't we clearing the pending request here http://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp#1381. What is teh actual scenario that we are looking we for?

Comment 16

[Vinay G Shetty]

And I don't think we are actually storing the not yet allowed requests anywhere as of now. Hence there is no question of clearing it. isn't it?

Comment 17

[Josh Matthews [:jdm]]

Right, that's why I suggested storing the list of cleared IDs and comparing against them when a request is Allowed.

Comment 18

[Vinay G Shetty]

Attachment: 886026.patch

I have added a condition where it tries to check if the clearWatch is already called for the given watchID, and if yes clearit again.

But I don't see any difference in test result. there is still a success callback.

Is it that the watch request is not getting stored in pending request at all?(sGeoInitPending == false)

Vinay G Shetty

Comment 19

[Vinay G Shetty]

Attachment: 886026.patch

Comment 20

[Vinay G Shetty]

Attachment: 886026.patch

Have fixed few warnings in the current patch. 

I ran the test_ClearWatch.html(not the one in the patch). Here is the result
TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | successCallback was called, hasBeenCleared=false, successWasCalledAfterClear=false
TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | clearWatch was called, hasBeenCleared=false, successWasCalledAfterClear=false
TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | testAccepted was called, hasBeenCleared=true, successWasCalledAfterClear=false

I mean, even here success was called before clearWatch tried to clear the watchRequest.

Is there a way that we can make watch request hang on for some time, so that ClearWatch has enough time to clear pendingrequest?

Comment 21

[Josh Matthews [:jdm]]

I don't understand what you're asking - test_ClearWatch.html intentionally waits for the first successful result before clearing the watch, and then checks that no more notifications arrive after the call to clearWatch.

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8513591 [details] [diff] [review]
886026.patch

Review of attachment 8513591 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty good!

::: dom/geolocation/nsGeolocation.cpp
@@ +1182,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +Geolocation::CheckIfAlreadyCleared(nsGeolocationRequest* aRequest)

IsAlreadyCleared

@@ +1185,5 @@
> +bool
> +Geolocation::CheckIfAlreadyCleared(nsGeolocationRequest* aRequest)
> +{
> +    for (uint32_t i = 0, length = mClearedWatchIDs.Length(); i < length; ++i) {
> +    if (aRequest->IsWatch() &&

Let's move this check into the caller.

@@ +1189,5 @@
> +    if (aRequest->IsWatch() &&
> +        (mClearedWatchIDs[i] == aRequest->WatchId())) {
> +              return true;
> +    }
> +    else{

This else branch doesn't make sense; we'll only check the first element of the array before we decide to return.

@@ +1368,5 @@
>    if (!nsContentUtils::IsCallerChrome()) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if(CheckIfAlreadyCleared(aRequest)){

nit: space before (, and check that the request is a watch here.

@@ +1369,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if(CheckIfAlreadyCleared(aRequest)){
> +      ClearWatch(aRequest->WatchId());

This won't work correctly, since NotifyAllowedRequest has not been called yet so the request is not in mWatchingCallbacks. I think the best thing to do is call NotifyAllowedRequest explicitly before ClearWatch.

@@ +1371,5 @@
>  
> +  if(CheckIfAlreadyCleared(aRequest)){
> +      ClearWatch(aRequest->WatchId());
> +  }
> +  else{

nit: else on the same line as }, space before {

::: dom/geolocation/nsGeolocation.h
@@ +153,5 @@
>  
>    // Remove request from all callbacks arrays
>    void RemoveRequest(nsGeolocationRequest* request);
>  
> +  //Check if clearWatch is already called

nit: space after //

@@ +211,5 @@
>  
>    // Pending requests are used when the service is not ready
>    nsTArray<nsRefPtr<nsGeolocationRequest> > mPendingRequests;
> +
> +  nsTArray<int32_t> mClearedWatchIDs;

Document this member, please.

::: dom/tests/mochitest/geolocation/mochitest.ini
@@ +47,5 @@
>  [test_windowClose.html]
>  skip-if = buildapp == 'b2g' || toolkit == 'android' #TIMED_OUT
>  [test_worseAccuracyDoesNotBlockCallback.html]
>  skip-if = buildapp == 'b2g' || toolkit == 'android' #TIMED_OUT
> +[test_clearWatchBeforeAllowing.html]

Add this in alphabetical order, please.

::: dom/tests/mochitest/geolocation/test_clearWatchBeforeAllowing.html
@@ +35,5 @@
> +    }, function(err) {
> +      errorCallbackCalled = true;
> +    }
> +  );
> +  

nit: trailing whitespace

@@ +37,5 @@
> +    }
> +  );
> +  
> +  navigator.geolocation.clearWatch(watchId);
> +  

nit: trailing whitespace

@@ +40,5 @@
> +  navigator.geolocation.clearWatch(watchId);
> +  
> +  navigator.geolocation.getCurrentPosition(
> +    function(pos) {
> +	  SimpleTest.executeSoon(function() { 

nit: indentation and trailing whitespace

@@ +46,5 @@
> +         "getCurrentPosition : Success callback should not have been called");
> +
> +       ok(errorCallbackCalled == false,
> +         "getCurrentPosition : Error callback should not have been called");
> +		 

nit: trailing whitespace

@@ +47,5 @@
> +
> +       ok(errorCallbackCalled == false,
> +         "getCurrentPosition : Error callback should not have been called");
> +		 
> +	   SimpleTest.finish();

nit: indentation

@@ +48,5 @@
> +       ok(errorCallbackCalled == false,
> +         "getCurrentPosition : Error callback should not have been called");
> +		 
> +	   SimpleTest.finish();
> +	  });

nit: indentation (should match the S in SimpleTest)

@@ +49,5 @@
> +         "getCurrentPosition : Error callback should not have been called");
> +		 
> +	   SimpleTest.finish();
> +	  });
> +	}

nit: indentation (should match the f in function)

@@ +53,5 @@
> +	}
> +  );
> +  /* XXX This relies on the runnable that allows
> +   * the request not being executed between these calls.
> +   */

This comment can go away.

@@ +64,5 @@
> +//    ok(errorCallbackCalled == false,
> +//       "Error callback should not have been called");
> +
> +//    SimpleTest.finish();
> +//  }, 10000);

Remove all of these commented lines.

Comment 23

[Vinay G Shetty]

Comment on attachment 8513591 [details] [diff] [review]
886026.patch

Review of attachment 8513591 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/nsGeolocation.cpp
@@ +1369,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if(CheckIfAlreadyCleared(aRequest)){
> +      ClearWatch(aRequest->WatchId());

Here ClearWatch is called only if the CheckIfAlreadyCleared returns true; which means that there is already a request in the PendingCallBack array isn't it?

why do u expect it to be added to mWatchingCallbacks as well?

Isn't this bug bothered only about the pending request?

Comment 24

[Josh Matthews [:jdm]]

If you read the implementation of ClearWatch, it only works if the request is present in mWatchingCallbacks. The only place this array is modified is in NotifyAllowRequest, which is called from Allow.

Comment 25

[Vinay G Shetty]

Attachment: 886026.patch

the allow being called after the ClearWatch request. hence the clearWatch request was ignored.

I am making a check in the Allow to see if there is already a clearwatch request that has been made on the current request based on a mClearedWatchIDs.

Comment 26

[Josh Matthews [:jdm]]

Comment on attachment 8518325 [details] [diff] [review]
886026.patch

Review of attachment 8518325 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good! There are a couple stylistic changes, and a couple bits of code to rearrange, but it's quite close! Please add a commit message, too :)

::: dom/geolocation/nsGeolocation.cpp
@@ +416,5 @@
>    MOZ_ASSERT(aChoices.isUndefined());
>  
> +  // Check if there is already ClearWatch called for current
> +  // request & clear if yes
> +  if (this->IsWatch() && mLocator->IsAlreadyCleared(this)) {

We should check this in Cancel, too, otherwise the error callback could be called on a canceled watch. Let's move this code to a new method and call it from both places.

@@ +1192,5 @@
>  
> +bool
> +Geolocation::IsAlreadyCleared(nsGeolocationRequest* aRequest)
> +{
> +    for (uint32_t i = 0, length = mClearedWatchIDs.Length(); i < length; ++i) {

nit: indentation

@@ +1194,5 @@
> +Geolocation::IsAlreadyCleared(nsGeolocationRequest* aRequest)
> +{
> +    for (uint32_t i = 0, length = mClearedWatchIDs.Length(); i < length; ++i) {
> +    if (mClearedWatchIDs[i] == aRequest->WatchId()) {
> +              return true;

nit: indentation

@@ +1380,5 @@
>  
>  NS_IMETHODIMP
>  Geolocation::ClearWatch(int32_t aWatchId)
>  {
> +  if (!mClearedWatchIDs.Contains(aWatchId)) {

Let's move this after the aWatchId < 0 check.

::: dom/tests/mochitest/geolocation/test_clearWatchBeforeAllowing.html
@@ +26,5 @@
> +});
> +
> +function run_test() {
> +  var successCallbackCalled = false,
> +      errorCallbackCalled = false;

nit: indentation

@@ +45,5 @@
> +
> +       ok(errorCallbackCalled == false,
> +         "getCurrentPosition : Error callback should not have been called");
> +
> +     SimpleTest.finish();

nit: indentation

@@ +47,5 @@
> +         "getCurrentPosition : Error callback should not have been called");
> +
> +     SimpleTest.finish();
> +      });
> +    }

nit: indentation

Comment 27

[Vinay G Shetty]

Attachment: 886026.patch

I have made the new method name as ClearPendingRequest. I thought the name would match its intentions. :)

Comment 28

[Josh Matthews [:jdm]]

Comment on attachment 8518961 [details] [diff] [review]
886026.patch

Review of attachment 8518961 [details] [diff] [review]:
-----------------------------------------------------------------

Super close!

::: dom/geolocation/nsGeolocation.cpp
@@ +1202,5 @@
> +{
> +  if (aRequest->IsWatch() && this->IsAlreadyCleared(aRequest)) {
> +    this->NotifyAllowedRequest(aRequest);
> +    this->ClearWatch(aRequest->WatchId());
> +    return;

This is almost going to work, but now we don't early-return from Allow and Cancel if we clear the request. This method should return a boolean and the callers should act on that.

::: dom/geolocation/nsGeolocation.h
@@ +154,5 @@
>    // Remove request from all callbacks arrays
>    void RemoveRequest(nsGeolocationRequest* request);
>  
> +  // Check if clearWatch is already called
> +  bool IsAlreadyCleared(nsGeolocationRequest* aRequest);

This can be private instead of public now.

::: dom/tests/mochitest/geolocation/test_clearWatchBeforeAllowing.html
@@ +37,5 @@
> +    }
> +  );
> +
> +  navigator.geolocation.getCurrentPosition(
> +   function(pos) {

nit: identation (should be two spaces)

@@ +45,5 @@
> +
> +       ok(errorCallbackCalled == false,
> +         "getCurrentPosition : Error callback should not have been called");
> +
> +	   SimpleTest.finish();

nit: indentation.

@@ +47,5 @@
> +         "getCurrentPosition : Error callback should not have been called");
> +
> +	   SimpleTest.finish();
> +     });
> +   }

nit: indentation (should be two spaces)

Comment 29

[Vinay G Shetty]

Comment on attachment 8518961 [details] [diff] [review]
886026.patch

Review of attachment 8518961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/nsGeolocation.cpp
@@ +1202,5 @@
> +{
> +  if (aRequest->IsWatch() && this->IsAlreadyCleared(aRequest)) {
> +    this->NotifyAllowedRequest(aRequest);
> +    this->ClearWatch(aRequest->WatchId());
> +    return;

what branching statement in the caller should I make based on the boolean return value?.
Or for now can I just ignore the return value in the caller?

Comment 30

[Josh Matthews [:jdm]]

You want to return a value that the caller checks and can decide whether to early-return or not. Just preserve the behaviour from your previous patch that didn't use the separate method; running the test should be able to show if this is working or not.

Comment 31

[Vinay G Shetty]

Attachment: 886026.patch

Made the ClearPendingRequest return bool value and further changes accordingly.

Comment 32

[Josh Matthews [:jdm]]

Comment on attachment 8519020 [details] [diff] [review]
886026.patch

Review of attachment 8519020 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry to be so fiddly about the indentation; I hope my requests are clear this time!

::: dom/geolocation/nsGeolocation.cpp
@@ +1193,5 @@
> +bool
> +Geolocation::IsAlreadyCleared(nsGeolocationRequest* aRequest)
> +{
> +  for (uint32_t i = 0, length = mClearedWatchIDs.Length(); i < length; ++i) {
> +   if (mClearedWatchIDs[i] == aRequest->WatchId()) {

nit: two space indents

@@ +1195,5 @@
> +{
> +  for (uint32_t i = 0, length = mClearedWatchIDs.Length(); i < length; ++i) {
> +   if (mClearedWatchIDs[i] == aRequest->WatchId()) {
> +     return true;
> +   }

nit: two space indents

::: dom/tests/mochitest/geolocation/test_clearWatchBeforeAllowing.html
@@ +37,5 @@
> +    }
> +  );
> +
> +  navigator.geolocation.getCurrentPosition(
> +  function(pos) {

nit: indent this by two spaces (and everything under it)

@@ +47,5 @@
> +         "getCurrentPosition : Error callback should not have been called");
> +
> +       SimpleTest.finish();
> +     });
> +  }

nit: indent this by two spaces

Comment 33

[Vinay G Shetty]

Attachment: 886026.patch

I guess i have corrected them as you sexpected :)

Comment 34

[Vinay G Shetty]

Attachment: 886026.patch

Updated few more changes

Comment 35

[Josh Matthews [:jdm]]

Comment on attachment 8519060 [details] [diff] [review]
886026.patch

Review of attachment 8519060 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 36

[Vinay G Shetty]

https://tbpl.mozilla.org/?tree=Try&rev=51e1f39b133a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/980c6c2b694c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/980c6c2b694c