Closed Bug 886026 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox24 --- affected
firefox25 --- affected

People

(Reporter: jdm, Assigned: shettyvinayg, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 8 obsolete files)

I suspect we'll assert in debug builds and not cancel the eventual watch in release ones.
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...
Queuing the pending requests is reasonable; I don't want web authors to have to worry about reissuing requests and hoping for the best.
Summary: See what happens if we clear a watch while the request is pending → Make clearing pending and not-yet-allowed watch requests work
I suppose we can store an array of cleared watch IDs and check it whenever Allow is called.
Whiteboard: [mentor=jdm][lang=js] → [mentor=jdm][lang=c++]
OS: Linux → All
Hardware: x86_64 → All
Attached patch Test case (obsolete) — Splinter Review
A test case for clearing not-yet-allowed requests.
(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.
Note that I said IDs, not requests.
By the way, there's a new contributor who appears to be looking into this bug, so please hold off on solving it :)
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 ?
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.
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 ?
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.
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.
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)?
Ah, no I was not.  I'll rebuild and check again.  Thanks!
Mentor: josh
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]
Assignee: nobody → shettyvinayg
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?
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?
Right, that's why I suggested storing the list of cleared IDs and comparing against them when a request is Allowed.
Attached patch 886026.patch (obsolete) — Splinter Review
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
Attachment #8513398 - Flags: review?(josh)
Attached patch 886026.patch (obsolete) — Splinter Review
Attachment #8513398 - Attachment is obsolete: true
Attachment #8513398 - Flags: review?(josh)
Attached patch 886026.patch (obsolete) — Splinter Review
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?
Attachment #8513586 - Attachment is obsolete: true
Attachment #8513591 - Flags: review?(josh)
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 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.
Attachment #8513591 - Flags: review?(josh) → feedback+
Flags: needinfo?(josh)
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?
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.
Flags: needinfo?(josh)
Attached patch 886026.patch (obsolete) — Splinter Review
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.
Attachment #8513591 - Attachment is obsolete: true
Attachment #8518325 - Flags: review?(josh)
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
Attachment #8518325 - Flags: review?(josh) → feedback+
Attached patch 886026.patch (obsolete) — Splinter Review
I have made the new method name as ClearPendingRequest. I thought the name would match its intentions. :)
Attachment #8518325 - Attachment is obsolete: true
Attachment #8518961 - Flags: review?(josh)
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)
Attachment #8518961 - Flags: review?(josh) → feedback+
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?
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.
Attached patch 886026.patch (obsolete) — Splinter Review
Made the ClearPendingRequest return bool value and further changes accordingly.
Attachment #8518961 - Attachment is obsolete: true
Attachment #8519020 - Flags: review?(josh)
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
Attachment #8519020 - Flags: review?(josh) → feedback+
Attached patch 886026.patch (obsolete) — Splinter Review
I guess i have corrected them as you sexpected :)
Attachment #8519020 - Attachment is obsolete: true
Attachment #8519051 - Flags: review?(josh)
Attached patch 886026.patchSplinter Review
Updated few more changes
Attachment #8519051 - Attachment is obsolete: true
Attachment #8519051 - Flags: review?(josh)
Attachment #8519060 - Flags: review?(josh)
Comment on attachment 8519060 [details] [diff] [review]
886026.patch

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

\o/
Attachment #8519060 - Flags: review?(josh) → review+
Keywords: checkin-needed
Attachment #767288 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/980c6c2b694c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=c++] → [lang=c++][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/980c6c2b694c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=c++][fixed-in-fx-team] → [lang=c++]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.