[OS.File] Expose temporary directory, profile directory

RESOLVED FIXED in mozilla17

Status

()

enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Writing tests or benchmarks without knowing the name of the temporary directory is tedious. I suspect that profile directory will become necessary pretty soon, too.

Let's fix this.
Assignee: nobody → dteller
Attachment #644282 - Flags: review?(khuey)
Comment on attachment 644282 [details] [diff] [review]
Exposing path to tmpdir, profiledir in OS.Constants

Review of attachment 644282 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +94,5 @@
>    }
>  
>    gInitialized = true;
>  
> +  ScopedDeletePtr<Paths> paths(new Paths);

Just use nsAutoPtr.  I know that ScopedDeletePtr does the same thing, but we don't currently use that in Gecko.

@@ +482,5 @@
> +{
> +    JSString* strValue = JS_NewUCStringCopyZ(cx, aValue.get());
> +    jsval valValue = STRING_TO_JSVAL(strValue);
> +    return JS_SetProperty(cx, aObject, aProperty, &valValue);
> +}

Two space indenting to fit with the rest of the file please.
Attachment #644282 - Flags: review?(khuey) → review+
Done. Although I would have preferred keeping Scoped :)
Attachment #644282 - Attachment is obsolete: true
Attachment #646123 - Flags: review+
Assignee: dteller → nobody
Component: Networking: File → OS.File
Keywords: checkin-needed
Product: Core → Toolkit
Assignee: nobody → dteller
Yoric, haven't we done this before? :P
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out for build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/154d7c224af2

https://tbpl.mozilla.org/php/getParsedLog.php?id=13890961&tree=Mozilla-Inbound

/usr/local/bin/ccache /builds/slave/m-in-osx64/build/clang/bin/clang++ -arch i386 -o OSFileConstants.o -c  -fvisibility=hidden -DDLL_PREFIX=\"lib\" -DDLL_SUFFIX=\".dylib\" -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_MACOSX=1 -DOS_POSIX=1  -I/builds/slave/m-in-osx64/build/content/events/src -I/builds/slave/m-in-osx64/build/dom/base -I/builds/slave/m-in-osx64/build/dom/bindings  -I/builds/slave/m-in-osx64/build/content/events/src -I/builds/slave/m-in-osx64/build/ipc/chromium/src -I/builds/slave/m-in-osx64/build/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/builds/slave/m-in-osx64/build/dom/system -I. -I../../dist/include  -I/builds/slave/m-in-osx64/build/obj-firefox/i386/dist/include/nspr -I/builds/slave/m-in-osx64/build/obj-firefox/i386/dist/include/nss      -fPIC -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fno-omit-frame-pointer   -Qunused-arguments  -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/OSFileConstants.o.pp /builds/slave/m-in-osx64/build/dom/system/OSFileConstants.cpp
../../../../dom/system/OSFileConstants.cpp:102:40: error: use of undeclared identifier 'aKey'
  nsresult rv = NS_GetSpecialDirectory(aKey, getter_AddRefs(file));
                                       ^
../../../../dom/system/OSFileConstants.cpp:105:12: error: redefinition of 'rv'
  nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(file));
           ^
../../../../dom/system/OSFileConstants.cpp:102:12: note: previous definition is here
  nsresult rv = NS_GetSpecialDirectory(aKey, getter_AddRefs(file));
           ^
2 errors generated.
Gasp, sorry.
Sorry about that, uploaded from the wrong repo again :/
Kyle, I took the opportunity to:
- add a few directories (homedir, desktopdir, libdir);
- pass the name of directories to camelCase.

Is that ok with you?
Attachment #646123 - Attachment is obsolete: true
Attachment #647156 - Flags: review?(khuey)
Comment on attachment 647156 [details] [diff] [review]
Exposing path to tmpdir, profiledir in OS.Constants, v2

Review of attachment 647156 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +140,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  gPaths = paths.forget();

paths.forget(&gPaths);
Attachment #647156 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Comment on attachment 647156 [details] [diff] [review]
> Exposing path to tmpdir, profiledir in OS.Constants, v2
> 
> Review of attachment 647156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/OSFileConstants.cpp
> @@ +140,5 @@
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> > +
> > +  gPaths = paths.forget();
> 
> paths.forget(&gPaths);

For some reason, this does not work:

/Users/david/Documents/Code/mozilla-central/dom/system/OSFileConstants.cpp:132: error: no matching function for call to ‘nsAutoPtr<mozilla::<unnamed>::Paths>::forget(mozilla::<unnamed>::Paths**)’

I guess gcc might be confused by the anonymous namespace.
Same code, but with an additional check to ensure that precompile_cache.js (or other scripts that ignore initialization errors) does not segfault.

By the way, I have just double-checked, nsAutoPtr does not have a method |forget(T**)|, it is nsRefPtr that has one. Which makes sense, since the only reason for this method to exist is to optimize away some reference counting.
Attachment #647156 - Attachment is obsolete: true
Attachment #649581 - Flags: review?(khuey)
> By the way, I have just double-checked, nsAutoPtr does not have a method
> |forget(T**)|, it is nsRefPtr that has one. Which makes sense, since the
> only reason for this method to exist is to optimize away some reference
> counting.

My bad.
Comment on attachment 649581 [details] [diff] [review]
Exposing path to tmpdir, profiledir in OS.Constants

Review of attachment 649581 [details] [diff] [review]:
-----------------------------------------------------------------

Remove the printf_stderr calls.
Attachment #649581 - Flags: review?(khuey) → review+
(In reply to Ryan VanderMeulen from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1844cc5b131a
> 
> I'll ask again - should this have tests?

Tests will arrive with another bug.
It's important to distinguish between the roaming profile directory and the local one, and so we should add both any time we add one of them, or else people will start to use the wrong one without realizing the difference.
I have never seen that even mentioned in any piece of documentation. I assume that's ProfD vs. ProfLD, but I am not clear about the difference.
Yes, and the roaming profile directory is where you keep files which should persist even as somebody moves from one Windows computer to another in a workgroup setting. The local profile directory is where we should keep large caches (network cache, urlclassifier cache etc) which are large and should not be moved across network shares.
Flags: in-testsuite? → in-testsuite-
Filed a followup bug for this purpose: bug 781346.
This was backed out due to Linux Crashtest IPC failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd4f4c909104

https://tbpl.mozilla.org/php/getParsedLog.php?id=14239198&tree=Mozilla-Inbound

REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/reftest/tests/content/base/crashtests/700512.html | 105 / 2103 (4%)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/content/base/crashtests/700512.html | load failed: timed out waiting for reftest-wait to be removed
REFTEST INFO | Saved log: START file:///home/cltbld/talos-slave/test/build/reftest/tests/content/base/crashtests/700512.html
REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering WaitForTestEnd
REFTEST INFO | Saved log: [CONTENT] WaitForTestEnd: Adding listeners
REFTEST INFO | Saved log: Initializing canvas snapshot
REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
REFTEST INFO | Saved log: [CONTENT] MakeProgress: dispatching MozReftestInvalidate
REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for reftest-wait to be removed
REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///home/cltbld/talos-slave/test/build/reftest/tests/content/base/crashtests/700512.html
REFTEST INFO | Saved log: Updating canvas for invalidation
REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for reftest-wait to be removed
REFTEST INFO | Loading a blank page
Investigating
Ok, I can reproduce the failure on TryServer. Unfortunately, I cannot reproduce it locally, so debugging will be a little complicated, especially given the current state of TryServer.

Investigating with instrumented code.
I will take the opportunity to add a testsuite. Self-reviewed (it's basically a two liner).
Attachment #651183 - Flags: review+
Posted patch Candidate fix (obsolete) — Splinter Review
This should fix the mysterious crashtest timeout. Currently on TryServer.

For reference, here is my understanding of the issue:
- the crashtest seems to launch not Firefox but rather XulRunner or some minimal embedding of Gecko which does not define special directory ProfD;
- since ProfD is not defined, initialization of my component fails, which should lead to an error in the initialization of ChromeWorkers;
- however, for some reason, the client of the RuntimeService ignores the error.

This patch ensures that the OSFileService does not fail if ProfD (or some other special directory) is not defined.
Attachment #651185 - Flags: review?(khuey)
Attachment #651185 - Attachment is obsolete: true
Attachment #653787 - Flags: review+
You need to log in before you can comment on or make changes to this bug.