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

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Remus Pop (:RemusPop)]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 2

[Remus Pop (:RemusPop)]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 4

[Remus Pop (:RemusPop)]

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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17682075

Comment 7

[Remus Pop (:RemusPop)]

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

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, my build process is working again. So I can check for this regression now.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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");

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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.

Comment 11

[Remus Pop (:RemusPop)]

Actually the geolocation tokens disappeared for normal browsing too, not just private browsing.

Comment 12

[Doug Turner (:dougt)]

> 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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 14

[Doug Turner (:dougt)]

> 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.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 16

[u279076]

(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.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 18

[Doug Turner (:dougt)]

We could write one.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 20

[Doug Turner (:dougt)]

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.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 22

[Doug Turner (:dougt)]

Attachment: 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

Comment 23

[Remus Pop (:RemusPop)]

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?

Comment 24

[Doug Turner (:dougt)]

i can add token support.  :)

Grab it here.

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

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 26

[Remus Pop (:RemusPop)]

Henrik I would gladly pass this to you so we get this faster.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 28

[Remus Pop (:RemusPop)]

Bug filed and dependency set.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Remus, can you please disable the token test for the time being, so we can make this test pass again?

Comment 30

[Remus Pop (:RemusPop)]

Attachment: skip pref geo token check (default)

This skips the check for a token stored in prefs and includes Henrik's patch.

Comment 31

[u279076]

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.

Comment 32

[u279076]

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.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 34

[Remus Pop (:RemusPop)]

Attachment: skip token check

Commented out token check.

Comment 35

[u279076]

Attachment: 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

Comment 36

[u279076]

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)

Comment 37

[u279076]

Please check tomorrow's dashboard and mark VERIFIED if this failure no longer occurs.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 39

[u279076]

(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.

Comment 40

[Andrei Eftimie]

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

Not sure about the Resolved flag.