Failure in /testPrivateBrowsing/testGeolocation.js | controller.assertJS: Failed for 'subject.hasGeoTokens == true'

RESOLVED INVALID

Status

P2
normal
RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: remus.pop, Unassigned)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
TEST: testPrivateBrowsing/testGeolocation.js::testTabRestoration
ERROR: controller.assertJS: Failed for 'subject.hasGeoTokens == true' 
WHEN: 2011-08-30
FIRST: 2011-08-20
FREQ: 41
BRANCH: default

This happens because in nightly we do not add/change a preference (in about:config) for a site that we allow to use geolocation on us. The preference is set only in release, beta and aurora.
Which preference are we talking about here? Do you have any information why we don't do this in nightly builds? For me it's suspicious.
(Reporter)

Comment 2

7 years ago
geo.wifi.access_token is not present in nightly when we allow a site to geolocate us.
i have no other information until now. It must be investigated because our test relies on this.
So is that a recent change in Nightly which has been caused a regression in our test? Does a build from Aug. 19th also show this issue?
(Reporter)

Comment 4

7 years ago
Found the regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f69a10f23bf3&tochange=1881f9b5f8b5

It appeared between the 18th and 19th of August nightly builds.
Remus, can you please also check the hourly builds? This range has a lot of commits too and it would be nice to be able to narrow it down even further. Thanks.

Comment 6

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17682075
(Reporter)

Comment 7

7 years ago
Build IDs for regression range narrowing.

OK : Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110819 Firefox/9.0a1 ID:20110819021046
NOK: Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110819 Firefox/9.0a1 ID:20110819024344
Ok, my build process is working again. So I can check for this regression now.
This is a regression from the switch to the Google Geolocation Service v2 which has been checked-in on bug 677256.

Doug, when a position has been returned by the old API we got a geo access token. Is that not the case anymore in Private Browsing mode? What happened with the token? For now we are checking for the prefbranch 'geo.wifi.access_token' and check for childs:

prefs.preferences.prefBranch.getChildList(PREF_GEO_TOKEN, tokens);
assert.notEqual(tokens.value, 0, "Geolocation tokens have to be present");
Blocks: 677256
Keywords: regression
Hardware: x86 → All
Created attachment 557480 [details] [diff] [review]
Patch (more stable and verbose messages)

Just a patch we should check-in later once we have figured out the problem on this bug and finally fix the test.
(Reporter)

Comment 11

7 years ago
Actually the geolocation tokens disappeared for normal browsing too, not just private browsing.
> when a position has been returned by the old API we got a geo access token. Is that not the case anymore in Private Browsing mode?

We may or may not get a token.

> What happened with the token?

We still support the the token
(In reply to Doug Turner (:dougt) from comment #12)
> > when a position has been returned by the old API we got a geo access token. Is that not the case anymore in Private Browsing mode?
> 
> We may or may not get a token.

So what is the logic behind? When do we get a token and when not? Is there any documentation or spec?
> So what is the logic behind? 

note sure what you mean.

> When do we get a token and when not?

It is random.  Up to the location provider.

> Is there any documentation or spec?

No.  Just the code.
(In reply to Doug Turner (:dougt) from comment #14)
> > So what is the logic behind? 
> 
> note sure what you mean.
> 
> > When do we get a token and when not?
> 
> It is random.  Up to the location provider.

So it means we should better remove the extra check for the token in our test? Given that it is random we can't really trust on its existence.
(In reply to Henrik Skupin (:whimboo) from comment #15)
> So it means we should better remove the extra check for the token in our
> test? Given that it is random we can't really trust on its existence.

If so, I would prefer to branch the Geo Token testing into its own Litmus test which is "not-doable" in Mozmill.
Well, you cannot do that as long as you do not know which provider returns a token. Once we know that, we would also be able to test this with Mozmill.

Doug, do you know a provider which returns a token?
We could write one.
(In reply to Doug Turner (:dougt) from comment #18)
> We could write one.

Do you have any links to some documentation I could use to write such a provider? That would help a lot. Thanks.
well, the the code that reads this content is all in NetworkGeolocation.js.  I'd also look at http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/network_geolocation.sjs?force=1.

If you want me to put something together, let me know.
Doug, given the current chemspill situation it would be great if you could put something together we could use for testing. Thanks for your offer.
Created attachment 558430 [details]
geofake

node ./geofake.js

then change geo.wifi.uri to:  http://127.0.0.1:8000


if you want to do testing, you can 127.0.0.1:1337/?lat=123&lon=456&acc=789

lat == latitude
lon == longitude
acc == accuracy
(Reporter)

Comment 23

7 years ago
Not sure if we could use this in our testruns.

I tried this locally and it did't store any tokens. Maybe I haven't used this right?
i can add token support.  :)

Grab it here.

https://github.com/dougt/geofake-server

Updated

7 years ago
Attachment #558430 - Attachment is obsolete: true
Thanks Doug. So in our case we do not have a node.js server and can't easily integrate it into our Mozmill harness. That means we would have to port the code to PHP and upload it to mozqa.com. Remus, are you familiar with PHP? If not I can work on it.
(Reporter)

Comment 26

7 years ago
Henrik I would gladly pass this to you so we get this faster.
Ok, I will take care. Could you file a bug under Infrastructure and assign it to me? That would be great.

Otherwise I would tend to say lets comment out the line for the token check until we have the final patch ready. I'm happy to review and land the patch once available.
(Reporter)

Updated

7 years ago
Depends on: 685805
(Reporter)

Comment 28

7 years ago
Bug filed and dependency set.
Remus, can you please disable the token test for the time being, so we can make this test pass again?
(Reporter)

Comment 30

7 years ago
Created attachment 561135 [details] [diff] [review]
skip pref geo token check (default)

This skips the check for a token stored in prefs and includes Henrik's patch.
Attachment #561135 - Flags: review?(hskupin)
Comment on attachment 561135 [details] [diff] [review]
skip pref geo token check (default)

I'm taking this review. Remus, please use me as the reviewer for changes made to tests.
Attachment #561135 - Flags: review?(hskupin) → review?(anthony.s.hughes)
Comment on attachment 561135 [details] [diff] [review]
skip pref geo token check (default)

>-var setupModule = function(module)
>-{
>-  controller = mozmill.getBrowserController();
>+function setupModule(module) {
>+  module.controller = mozmill.getBrowserController();
>+  module.tabBrowser = new tabs.tabBrowser(controller);
> 
>-  pb = new privateBrowsing.privateBrowsing(controller);
>-  tabBrowser = new tabs.tabBrowser(controller);
>+  // Make sure we are not in PB mode and don't show a prompt
>+  module.pb = new privateBrowsing.privateBrowsing(controller);
>+  module.pb.enabled = false;
>+  module.pb.showPrompt = false;
> }
> 
>-var teardownModule = function(module)
>-{
>-  pb.reset();
>+function teardownModule(module) {
>+  module.pb.reset();
> }

Using module in your function calls and variable declarations is not necessary.

>-  // Make sure we are not in PB mode and don't show a prompt
>-  pb.enabled = false;
>-  pb.showPrompt = false;
>-

Why have you removed this check?

>   // Start Private Browsing
>   pb.start();
> 
>   // Load a page which supports geolocation and accept sharing the location
>   controller.open(localTestFolder + "geolocation/position.html");
>   controller.waitForPageLoad();
> 
>   var shortcut = utils.getProperty("chrome://browser/locale/browser.properties",
>-                                      "geolocation.shareLocation.accesskey");
>+                                   "geolocation.shareLocation.accesskey");
>   controller.keypress(null, shortcut, {ctrlKey: mozmill.isMac, altKey: !mozmill.isMac});
> 
>   try {
>     var result = new elementslib.ID(controller.tabs.activeTab, "result");
>-    controller.waitForEval("subject.innerHTML != 'undefined'", gTimeout, 100,
>-                           result.getNode());
>+    controller.waitFor(function () {
>+      return typeof(result.getNode().innerHTML) !== undefined;
>+    }, "Geo location has been found.", undefined, TIMEOUT);
>     available = true;
>-  } catch (ex) {}
>+  }
>+  catch (ex if ex.name === 'TimeoutError') {
>+  }
> 
>+  /* XXX: skip checking for geolocation tokens - Bug 685805 
>+ 

Bug number should be at the front of the comment.
// XXX: Bug 685805 - comment
 
>   // If a position has been returned check for geo access tokens
>   if (available) {
>     prefs.preferences.prefBranch.getChildList(PREF_GEO_TOKEN, tokens);
>-    controller.assertJS("subject.hasGeoTokens == true",
>-                        {hasGeoTokens: tokens.value > 0});
>+    assert.notEqual(tokens.value, 0, "Geolocation tokens have to be present");
>   }

Are we supposed to be removing token checking altogether? If so, this code change is not complete.

I would also like us to not remove the code but simply comment it out. When we add token testing support we can uncomment the code instead of having to rewrite it from scratch.
Attachment #561135 - Flags: review?(anthony.s.hughes) → review-
Remus, please do not pull in the proposed changes from my patch attached on this bug. Simply disable this specific token test. Nothing more. We should keep patches as small and focused as possible.
(Reporter)

Comment 34

7 years ago
Created attachment 561395 [details] [diff] [review]
skip token check

Commented out token check.
Attachment #561135 - Attachment is obsolete: true
Attachment #561395 - Flags: review?(anthony.s.hughes)
Attachment #561395 - Flags: review?(anthony.s.hughes) → review+
Created attachment 561487 [details] [diff] [review]
Patch v2: skip token check (default) [checked-in]

Just a couple of nits from your patch which I changed before check-in:
 * no space needed between /* XXX and // comments
 * commit comment should include the bug number
Attachment #561395 - Attachment is obsolete: true
Attachment #561487 - Flags: review+
Comment on attachment 561487 [details] [diff] [review]
Patch v2: skip token check (default) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/0b546638ce0e (default)
Attachment #561487 - Attachment description: Patch v2: skip token check (default) → Patch v2: skip token check (default) [checked-in]
Please check tomorrow's dashboard and mark VERIFIED if this failure no longer occurs.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Anthony, this is not a fix only a temporary workaround until we can check again for a token. So the bug has to stay open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Henrik Skupin (:whimboo) from comment #38)
> Anthony, this is not a fix only a temporary workaround until we can check
> again for a token. So the bug has to stay open.

Oh, right. Sorry.
(Reporter)

Updated

6 years ago
Assignee: remus.pop → nobody
Priority: -- → P2

Comment 40

5 years ago
This test doesn't exist for a long time now.
Has been removed as part of bug 818456.

Not sure about the Resolved flag.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.