David Teller [:Yoric] - still alive but not very active Assignee
	Description • 12 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 3 • 12 years ago

Done. Although I would have preferred keeping Scoped :)

	Ryan VanderMeulen [:RyanVM]
	Comment 4 • 12 years ago

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

Should this have a test?

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 6 • 12 years ago

Gasp, sorry.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 7 • 12 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 8 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 9 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 10 • 12 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 11 • 12 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 12 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 13 • 12 years ago

Thanks

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 12 years ago

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

I'll ask again - should this have tests?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 15 • 12 years ago

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

	Benjamin Smedberg
	Comment 16 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 17 • 12 years ago

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.

	Benjamin Smedberg
	Comment 18 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 12 years ago

Filed a followup bug for this purpose: bug 781346.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 21 • 12 years ago

Investigating

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 22 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 23 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 24 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 25 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 28 • 12 years ago

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