775588 - [OS.File] Expose temporary directory, profile directory

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to tmpdir, profiledir in OS.Constants

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to tmpdir, profiledir in OS.Constants

Done. Although I would have preferred keeping Scoped :)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75e0a5a581bb

Should this have a test?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Gasp, sorry.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to tmpdir, profiledir in OS.Constants, v2

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?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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);

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to tmpdir, profiledir in OS.Constants

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.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> 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 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to tmpdir, profiledir in OS.Constants

Thanks

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1844cc5b131a

I'll ask again - should this have tests?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 16

[Benjamin Smedberg]

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.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Filed a followup bug for this purpose: bug 781346.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Investigating

Comment 22

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

I will take the opportunity to add a testsuite. Self-reviewed (it's basically a two liner).

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: Candidate fix

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.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=919735249991

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fix for bustage

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Green on Try. (The Windows m-oth orange was due to another patch in the push)
https://tbpl.mozilla.org/?tree=Try&rev=e0fee2e553ca

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a822acf8faa
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fefd087abc
https://hg.mozilla.org/integration/mozilla-inbound/rev/dba7f413d26b

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a822acf8faa
https://hg.mozilla.org/mozilla-central/rev/a9fefd087abc
https://hg.mozilla.org/mozilla-central/rev/dba7f413d26b