Closed Bug 995697 Opened 11 years ago Closed 10 years ago

Intermittent browser_geolocation_privatebrowsing_perwindowpb.js | Test timed out | Found a browser window after previous test timed out

Categories

(Core :: DOM: Geolocation, defect)

Product:
Core
Component:
DOM: Geolocation
Platform:
x86
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- fixed

People

(Reporter: RyanVM, Assigned: Gijs)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

MozReview Request: Bug 995697 - remove obsolete geolocation private browsing test, r?jdm
39 bytes, text/x-review-board-request
jdm
: review+
Details
https://tbpl.mozilla.org/php/getParsedLog.php?id=37699511&tree=Mozilla-Inbound WINNT 6.2 mozilla-inbound pgo test mochitest-browser-chrome-2 on 2014-04-12 14:51:23 PDT for push ba77af4dbd10 slave: t-w864-ix-077 15:00:46 INFO - TEST-START | chrome://mochitests/content/browser/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js 15:00:46 INFO - TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js | Got load 15:01:31 INFO - TEST-INFO | screenshot: exit status 0 15:01:31 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js | Test timed out 15:01:31 INFO - TEST-INFO | MEMORY STAT vsize after test: 738983936 15:01:31 INFO - TEST-INFO | MEMORY STAT vsizeMaxContiguous after test: 1893662720 15:01:31 INFO - TEST-INFO | MEMORY STAT residentFast after test: 154562560 15:01:31 INFO - TEST-INFO | MEMORY STAT heapAllocated after test: 54244974 15:01:31 INFO - INFO TEST-END | chrome://mochitests/content/browser/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js | finished in 45021ms 15:01:31 INFO - TEST-INFO | checking window state 15:01:31 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js | Found a browser window after previous test timed out
After a month without any timeouts, this test is now timing out about once per day.
David: this intermittent geolocation failure started up (again) after your try push in comment 25 for bug 1042889. The subsequent intermittent reports are all on fx-team. Have any of your changes for bug 1042889 landed in fx-team? Could they have caused network timeouts when connecting to Google's Location Service?
Flags: needinfo?(dkeeler)
Looking at the test, I see this is probably unrelated to bug 1042889. This test just talks to a local network_geolocation.sjs page, not Google's Location Service. Curiously, all these test timeouts happen on Windows.
Flags: needinfo?(dkeeler)
Garvan, I know this is an old bug, but do you think the recent changes to use the Windows 8 Location API (bug 512407) could break the geolocation tests that assume they can override GLS by setting the "geo.wifi.uri" pref?
Flags: needinfo?(gkeeley)
All the geo mochitests are tests of the network geolocation provider, with a fake web service (as you noted, the geo.wifi.url is overridden to point to this fake local service). There are no geo mochitests that are affected by other providers (such as the CoreLocation or Windows provider).
Flags: needinfo?(gkeeley)
BTW, this test is super-annoying in that it fails one a day, mostly on Windows. The quick-fix (which seems to have worked with other geo tests that were timing out) was to increase wait times for responses. IIRC, this test looked correctly event-driven to me, and I didn't see any quick fixes, and didn't spend further time on it. I should look at it again more closely when I have some time.
Chris one more comment on your question(In reply to Chris Peterson [:cpeterson] from comment #115) > Garvan, I know this is an old bug, but do you think the recent changes to > use the Windows 8 Location API (bug 512407) could break the geolocation > tests that assume they can override GLS by setting the "geo.wifi.uri" pref? Wouldn't this become non-intermittent if this was the case? In m-c's firefox.js, the "geo.provider.ms-windows-location" pref is not in that file. In the code, we have it defaulting to false: https://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp#817 #ifdef XP_WIN if (Preferences::GetBool("geo.provider.ms-windows-location", false)) { mProvider = new WindowsLocationProvider(); } #endif
This is failing more frequently. I just ran ./mach mochitest-browser --repeat 99 dom/tests/browser on Windows 8.1 and I am getting intermittent failures. Problem is, I get intermittent failures in other tests also, such as browser_test_new_window_from_content.js. Going to try this on my mac now and see if I get similar intermittent failures in other tests. Point being, this may be a larger systematic problem, and not specific to this test.
And yet more notices of failure. I suspect this was failing in early 2013, because this mysterious commit happened: http://hg.mozilla.org/mozilla-central/diff/d42a2a82f3d2/dom/tests/browser/browser_geolocation_privatebrowsing_perwindowpb.js Which has changes that are unclear to me as to their purpose. The code prior to that commit looks more understandable to me. An example is the change that adds toggling the "geo.wifi.scan" pref, which is not done in any of the many geo mochitests. Indeed NetworkGeolocationProvider will try to do a scan with the default setting, but this has never had an affect on the tests that I have seen, as is largely a no-op on the test machines. We could try to back the code prior to that commit. Not that this will necessarily fix this case which appears to be resource starvation on the test machine. But would make for a simpler-looking test case :).
Ehsan, you reviewed the commit I mentioned in comment 132. It was a while ago, but any chance you recall something about that change?
Flags: needinfo?(ehsan)
(In reply to Garvan Keeley [:garvank] from comment #264) > Ehsan, you reviewed the commit I mentioned in comment 132. It was a while > ago, but any chance you recall something about that change? Olli wrote that code.
Flags: needinfo?(ehsan) → needinfo?(bugs)
That change was about, IIRC, geolocation stuff crashing locally on linux. This bug started to happen way later SnowWhite patch landed.
Flags: needinfo?(bugs)
Can we please get a fresh look at this? It's still one of our top failures and hasn't had any activity in nearly two months.
Flags: needinfo?(gkeeley)
Depends on: 1169243
Josh, do you know what this test is testing? This response from the sjs appears incorrect in that it has an access-token property, which isn't part of html5 geo (not that I am aware of): https://dxr.mozilla.org/mozilla-central/source/dom/tests/browser/network_geolocation.sjs#30 The test itself seems to be checking tokens added to the URL, which I don't see what bearing that has to geo in a private browsing context.
Flags: needinfo?(josh)
Ah, the thing we cared about testing was removed in http://hg.mozilla.org/mozilla-central/rev/6c236b24064b . This test can totally be removed.
Flags: needinfo?(josh)
Bug 995697 - remove obsolete geolocation private browsing test, r?jdm
Attachment #8613491 - Flags: review?(josh)
Attachment #8613491 - Flags: review?(josh) → review+
Comment on attachment 8613491 [details] MozReview Request: Bug 995697 - remove obsolete geolocation private browsing test, r?jdm https://reviewboard.mozilla.org/r/9749/#review8547 Ship It!
network_geolocation.sjs can be removed also
Flags: needinfo?(gkeeley)
https://reviewboard.mozilla.org/r/9749/#review8549 ::: dom/tests/browser/browser.ini:5 (Diff revision 1) > network_geolocation.sjs This can be removed
(In reply to Garvan Keeley [:garvank] from comment #562) > https://reviewboard.mozilla.org/r/9749/#review8549 > > ::: dom/tests/browser/browser.ini:5 > (Diff revision 1) > > network_geolocation.sjs > > This can be removed This seems to be used outside of this directory?
Flags: needinfo?(gkeeley)
(In reply to :Gijs Kruitbosch from comment #563) > (In reply to Garvan Keeley [:garvank] from comment #562) > > https://reviewboard.mozilla.org/r/9749/#review8549 > > > > ::: dom/tests/browser/browser.ini:5 > > (Diff revision 1) > > > network_geolocation.sjs > > > > This can be removed > > This seems to be used outside of this directory? Are you asking if it is? Or saying that it is? I don't understand :).
Flags: needinfo?(gkeeley)
(In reply to Garvan Keeley [:garvank] from comment #564) > (In reply to :Gijs Kruitbosch from comment #563) > > (In reply to Garvan Keeley [:garvank] from comment #562) > > > https://reviewboard.mozilla.org/r/9749/#review8549 > > > > > > ::: dom/tests/browser/browser.ini:5 > > > (Diff revision 1) > > > > network_geolocation.sjs > > > > > > This can be removed > > > > This seems to be used outside of this directory? > > Are you asking if it is? Or saying that it is? I don't understand :). A little bit of both. I had checked MXR and it seemed like it was referenced in a bunch of places. Looking again, it seems like it has a doppelganger in dom/tests/mochitests which is the thing that other places still refer to. I'll update the patch before landing. Note that as far as this intermittent goes, at least for the win8 case I'm worried that we'll just end up turning some other test in this dir orange, but we'll see...
https://hg.mozilla.org/mozilla-central/rev/5f2044bd4f0c
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
> A little bit of both. I had checked MXR and it seemed like it was referenced > in a bunch of places. Looking again, it seems like it has a doppelganger in > dom/tests/mochitests which is the thing that other places still refer to. > I'll update the patch before landing. > Those are just named the same in other directories. The sjs is this directory is used only by this mochitest, anyhoo, I'll remove it now so I don't forget.
Nevermind, RyanVM removed the unused file, thanks Ryan.
Assignee: nobody → gijskruitbosch+bugs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: