Closed Bug 489585 Opened 15 years ago Closed 15 years ago

Private browsing cache test leaves directories behind

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.3a1

People

(Reporter: sgautherie, Assigned: ted)

References

Details

(Whiteboard: fixed by bug 459114)

Attachments

(1 obsolete file) 

The first one I looked into is
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_bug248970_cache.js
which do
|78   dir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, 0700);|

I tried to add |_Dir.remove(true);| at its end,
but this fails with
|[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsILocalFile.remove]"|.

It looks like (at least) |cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);| would be keeping some files open...

Is there a way to explicitly shutdown the cache service, or something?
Attached patch Patch (v1) [Backout: Comment 7] (obsolete) — — Splinter Review 
Simulate a shutdown at the end of the test to allow various services which use the profile directory to clean themselves up, and remove the profile directory in the end.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #374088 - Flags: superreview?(bzbarsky)
Attachment #374088 - Flags: review?(bzbarsky)
Keywords: helpwanted
Summary: Some tests leave (temp) directories behind → Private browsing cache test leaves directories behind
Comment on attachment 374088 [details] [diff] [review]
Patch (v1)
[Backout: Comment 7]

>+function remove_profile_dir() {

Thanks!

I wonder whether this function should take the dir in argument.

Or even better, could the dir removal stay in the test but the fake shutdown function move to <testing/xpcshell/head.js>.
I guess it could be used in other tests...
Ehsan, I don't think the bug summary change was wanted:
there are other tests I'll look into after this one!
(In reply to comment #2)
> (From update of attachment 374088 [details] [diff] [review])
> >+function remove_profile_dir() {
> 
> Thanks!
> 
> I wonder whether this function should take the dir in argument.

We need to store the dir globally somehow because we don't know its path in advance.  Given that, what would the benefit of this change be?

> Or even better, could the dir removal stay in the test but the fake shutdown
> function move to <testing/xpcshell/head.js>.
> I guess it could be used in other tests...

That's a good idea, but I think it falls out of the scope of this bug.  Can you please file a new bug for that and assign it to me?
Blocks: 489605
(In reply to comment #3)
> Ehsan, I don't think the bug summary change was wanted:
> there are other tests I'll look into after this one!

Oh, sorry I didn't understand this, because comment 0 only mentioned that problem, and I was perhaps too quick to fix it.  :-)

Guilty as charged, filed bug 489605 to act as a tracker for any future bugs you might want to file for these problems.  :-)
Component: General → Networking: Cache
QA Contact: general → networking.cache
Attachment #374088 - Flags: superreview?(bzbarsky)
Attachment #374088 - Flags: superreview+
Attachment #374088 - Flags: review?(bzbarsky)
Attachment #374088 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/88f76db49467
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Can someone with a Mac please try this patch?  I'm curious to see which files remain in the temporary directory created by this test after it's finished.
(In reply to comment #8)

Could be bug 473385...
Status: REOPENED → ASSIGNED
Depends on: 473385
(In reply to comment #7)
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240467249.1240470102.3268.gz&fulltext=1

{
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-macosx-unittest/build/objdir/_tests/xpcshell/test_necko/unit/test_bug248970_cache.js | test failed, see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/Xh/XhBjGXEcHs0yUSzvR91WE++++TM/-Tmp-/runxpcshelltests_leaks.log
*** test pending
*** test finished
*** exiting
*** PASS ***

  <<<<<<<
}

No reported error, then the test returned a non-zero return code...
(In reply to comment #10)
> No reported error, then the test returned a non-zero return code...

I filed bug 489840 to enhance the error message.
(In reply to comment #11)
> (In reply to comment #10)
> > No reported error, then the test returned a non-zero return code...
> 
> I filed bug 489840 to enhance the error message.

Great, thanks!  That should help us get some more information here; adding as a dependency.
Depends on: 489840
Serge, do you think we should land this again to see if we can get a useful error message this time now that bug 489840 has been fixed?

Boris, are you OK with that?
Is there a reason not to be trying things like that on the try server?
(In reply to comment #14)
> Is there a reason not to be trying things like that on the try server?

Not really.  I guess somewhere deep inside I'm still not used to the idea that the try server runs unit tests now.  :-)

Pushed a try server changeset.
Here is the result of the try server run:

TEST-UNEXPECTED-FAIL | /builds/buildbot/sendchange-slave/sendchange-mac-unittest/mozilla/objdir/_tests/xpcshell/test_necko/unit/test_bug248970_cache.js | test failed (with xpcshell return code: -10), see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-/runxpcshelltests_leaks.log
*** test pending
*** test finished
*** exiting

I'm not sure what this return value (-10) means though.
The latest results I have from tryserver-debugging this is that the remove_profile_dir call is executed successfully, and after that for some reason xpcshell aborts with an exit code of -10 (it may as well be a crash).  Further debugging this using the try server seems unpractical; this needs help from someone who has a Mac.

Unassigning from myself for now.
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
Did you try bug 473385 workaround?
(In reply to comment #19)
> Did you try bug 473385 workaround?

No, but my tests with the try server indicated that nsIFile::Remove was not throwing, so I doubt that this could be related to bug 473385.
I've got a mac.  If I run check-one on this test with this patch, I get a crash.  If I run check-interactive and attach gdb before running, then I get the test passing.  Then when I quit xpcshell, I get a crash:

(gdb) bt
#0  0x95da6584 in CFRelease ()
#1  0x00da8d6e in nsNetworkLinkService::Shutdown (this=0x8294b0) at /Users/bzbarsky/mozilla/vanilla/mozilla/netwerk/system/mac/nsNetworkLinkService.mm:160
#2  0x00da8ded in nsNetworkLinkService::Observe (this=0x8294b0, subject=0x810964, topic=0x6782b0 "xpcom-shutdown", data=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/netwerk/system/mac/nsNetworkLinkService.mm:84
#3  0x005d89bf in nsObserverList::NotifyObservers (this=0x8405a0, aSubject=0x810964, aTopic=0x6782b0 "xpcom-shutdown", someData=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/ds/nsObserverList.cpp:128
#4  0x005d9e94 in nsObserverService::NotifyObservers (this=0x823270, aSubject=0x810964, aTopic=0x6782b0 "xpcom-shutdown", someData=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/ds/nsObserverService.cpp:181
#5  0x005c82c4 in NS_ShutdownXPCOM_P (servMgr=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/build/nsXPComInit.cpp:764
#6  0x00030a49 in NS_ShutdownXPCOM (svcMgr=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/stub/nsXPComStub.cpp:175
#7  0x00005ea3 in main (argc=15, argv=0xbfffe974, envp=0xbfffe9b4) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/xpconnect/shell/xpcshell.cpp:1770
(gdb) frame 1
#1  0x00da8d6e in nsNetworkLinkService::Shutdown (this=0x8294b0) at /Users/bzbarsky/mozilla/vanilla/mozilla/netwerk/system/mac/nsNetworkLinkService.mm:160
160         ::CFRelease(mReachability);
(gdb) p mReachability
$1 = (const struct __SCNetworkReachability *) 0x0

It looks like the problem is that this code is getting xpcom-shutdown twice and isn't expecting that.

That said, this code is broken, I think: if Init() fails it's quite possible to get here with null mReachability.  It should be null-checking things before releasing them and before passing them to various stuff...

ccing patch authors and reviewers to sanity-check that claim.
Going to fix this more generally in bug 459114.
Depends on: 459114
This is now fixed by bug 459114.
Assignee: nobody → ted.mielczarek
Status: NEW → RESOLVED
Closed: 15 years ago → 15 years ago
No longer depends on: 473385
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: fixed by bug 459114
Attachment #374088 - Attachment description: Patch (v1) → Patch (v1) [Backout: Comment 7]
Attachment #374088 - Attachment is obsolete: true
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a2pre) Gecko/20090807 Minefield/3.6a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/744cffd14567 + ...)

V.Fixed
Blocks: PrivateBrowsing
Status: RESOLVED → VERIFIED
No longer depends on: PrivateBrowsing
Flags: in-testsuite+
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a1
(In reply to comment #4)
> > Or even better, could the dir removal stay in the test but the fake shutdown
> > function move to <testing/xpcshell/head.js>.
> 
> That's a good idea, but I think it falls out of the scope of this bug.  Can you
> please file a new bug for that and assign it to me?

I filed bug 511415.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: