We've got the following permanent orange showing up on our XPCShell tests: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/security/manager/ssl/tests/unit/test_sts_preloadlist.js | test failed (with xpcshell return code: 3), see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpAyQpHa/runxpcshelltests_leaks.log /home/cltbld/talos-slave/test/build/xpcshell/tests/security/manager/ssl/tests/unit/test_sts_preloadlist.js:6: TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../../mozilla/xpcom/base/nsExceptionService.cpp, line 166 WARNING: OOPDeinit() without successful OOPInit(): file ../../../../mozilla/toolkit/crashreporter/nsExceptionHandler.cpp, line 2219 nsStringStats => mAllocCount: 2231 => mReallocCount: 185 => mFreeCount: 2231 => mShareCount: 7613 => mAdoptCount: 78 => mAdoptFreeCount: 78 <<<<<<< Thunderbird doesn't have the private browsing component registered, so this is why the test is failing. The test should be altered to account for this possibility - example test: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/contentprefs/tests/unit/test_bug248970.js#12

Here's a proposed fix. mconley: can you check it out and let me know if it works for you?

Comment on attachment 655626 [details] [diff] [review] proposed patch Yes, this looks like it'll do the job.

Comment on attachment 655626 [details] [diff] [review] proposed patch Honza, can you do a quick review? The patch is pretty small, and I think you've recently seen this xpcshell test.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/807627473028

Unfortunately, this actually causes hangs now on Thunderbird's tests. The cause is that we're missing run_next_test() that should be at the end of each function added via add_test(). What I don't quite understand is how the test currently passes for Firefox.

This fixes the missing run_next_test calls. Given I'm not sure about the FF case, I've pushed this to try on one platform just to make sure: https://tbpl.mozilla.org/?tree=Try&rev=b32fbca7a5f7

We can't unconditionally call run_next_test() because if we are running the private browsing mode tests, we rely on an observer to notify when the private browsing transitions have completed.

(In reply to David Keeler from comment #16) > Created attachment 656075 [details] [diff] [review] > conditionally run_next_test > > We can't unconditionally call run_next_test() because if we are running the > private browsing mode tests, we rely on an observer to notify when the > private browsing transitions have completed. Sorry, but I don't understand this. Assuming you are using the globally defined for xpcshell-tests run_next_test, it states: * Each test function must call run_next_test() when it's done. Test files * should call run_next_test() in their run_test function to execute all * async tests. http://hg.mozilla.org/mozilla-central/annotate/271ca35d7645/testing/xpcshell/head.js#l857 So I believe the test as-is isn't working correctly. The actual route it takes, I think, is that once the first run_next_test is called, it queues the first test for running, but because of the do_execute_soon, it doesn't actually queue the test - and instead, run_test() gets to exit before that do_execute_soon activates, and because there's no extra do_test_pending/do_test_finished, the test just exists, because the pending count is 0. If I'm wrong, I'd be grateful if you could point me to where run_next_test() is being called for the private browsing tests, as that's the bit I'm currently missing.

This is where run_next_test() gets called: http://hg.mozilla.org/mozilla-central/file/271ca35d7645/security/manager/ssl/tests/unit/test_sts_preloadlist.js#l29 due to the observer on "private-browsing-transition-complete" (which gets triggered at the end of test_part1 and test_private_browsing1). The issue is we need to wait for that transition to complete before running the next test in each case.

Comment on attachment 655932 [details] [diff] [review] Fix missing run_next_test calls Thanks, somehow I'd missed that.

I've started a try run for firefox: https://tbpl.mozilla.org/?tree=Try&rev=9d16f4036117 Mark - I'm assuming the process for running the patch through thunderbird-try is similar, but I don't know the details. Would you mind taking care of that or telling me how?

(In reply to David Keeler from comment #20) > I've started a try run for firefox: > https://tbpl.mozilla.org/?tree=Try&rev=9d16f4036117 OT: I'm surprised that you didn't limit that just to xpcshell. > Mark - I'm assuming the process for running the patch through > thunderbird-try is similar, but I don't know the details. Would you mind > taking care of that or telling me how? I've done it here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b03244136e20 For future reference, instructions here: https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer

Ignore my previous try push, I'd forgotten to add the actual patch. This one includes the patch: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=46861c0a084b

(In reply to Mark Banner (:standard8) from comment #22) > Ignore my previous try push, I'd forgotten to add the actual patch. This one > includes the patch: > > https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=46861c0a084b Sorry, messed up the file name, one last push: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=0cb753b91d9b

(In reply to Mark Banner (:standard8) from comment #23) > https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=0cb753b91d9b Finally this was right and the tests have passed :-)

Thanks.

(In reply to Mark Banner (:standard8) from comment #21) > OT: I'm surprised that you didn't limit that just to xpcshell. d'oh! > I've done it here: > > https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b03244136e20 > > For future reference, instructions here: > https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer Thanks. This looks good - marking checkin-needed.

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd51f6ca80e2

Comment on attachment 655626 [details] [diff] [review] proposed patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 760307 - this causes a perma-orange unit test failure for applications that don't have the private browsing service installed. User impact if declined: None Testing completed (on m-c, etc.): Landed on m-c Risk to taking this patch (and alternatives if risky): None, test-only String or UUID changes made by this patch: None Both of the patches on this bug need to be accepted, however the statements are the same for both.

Comment on attachment 656075 [details] [diff] [review] conditionally run_next_test See previous comment.

Comment on attachment 655626 [details] [diff] [review] proposed patch [Triage Comment] Test-only fix approved for Aurora 17.

I transplanted these to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/cbb60055616c https://hg.mozilla.org/releases/mozilla-aurora/rev/085e6c6642d1