Closed Bug 785860 Opened 12 years ago Closed 12 years ago

Permanent orange: TEST-UNEXPECTED-FAIL | test_sts_preloadlist.js:6: TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
firefox17 --- fixed

People

(Reporter: mconley, Assigned: geekboy)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

proposed patch
4.67 KB, patch
briansmith
: review+
mconley
: feedback+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
conditionally run_next_test
1.81 KB, patch
briansmith
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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
Attached patch proposed patch — — Splinter Review 
Here's a proposed fix.  mconley: can you check it out and let me know if it works for you?
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #655626 - Flags: feedback?(mconley)
Comment on attachment 655626 [details] [diff] [review]
proposed patch

Yes, this looks like it'll do the job.
Attachment #655626 - Flags: feedback?(mconley) → feedback+
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.
Attachment #655626 - Flags: review?(honzab.moz)
Component: Testing Infrastructure → Networking
Product: Thunderbird → Core
Attachment #655626 - Flags: review?(honzab.moz) → review+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/807627473028
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/807627473028
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix missing run_next_test calls (obsolete) — — Splinter Review 
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
Attachment #655932 - Flags: review?(sstamm)
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.
Attachment #656075 - Flags: review?(bsmith)
(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.
Attachment #656075 - Flags: review?(bsmith) → review+
Comment on attachment 655932 [details] [diff] [review]
Fix missing run_next_test calls

Thanks, somehow I'd missed that.
Attachment #655932 - Attachment is obsolete: true
Attachment #655932 - Flags: review?(sstamm)
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.
Keywords: checkin-needed
Whiteboard: [tb-orange] → [tb-orange] [checkin-needed: conditionally run_next_test]
Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd51f6ca80e2
Keywords: checkin-needed
Whiteboard: [tb-orange] [checkin-needed: conditionally run_next_test] → [tb-orange]
https://hg.mozilla.org/mozilla-central/rev/cd51f6ca80e2
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
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.
Attachment #655626 - Flags: approval-mozilla-aurora?
Comment on attachment 656075 [details] [diff] [review]
conditionally run_next_test

See previous comment.
Attachment #656075 - Flags: approval-mozilla-aurora?
Comment on attachment 655626 [details] [diff] [review]
proposed patch

[Triage Comment]
Test-only fix approved for Aurora 17.
Attachment #655626 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656075 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [tb-orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: