Closed
Bug 810543
Opened 12 years ago
Closed 12 years ago
OS.Constants.Path.profileDir not available in xpcshell tests
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: gps, Assigned: Yoric)
References
Details
Attachments
(4 files, 11 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Cu.import("resource://gre/modules/osfile.jsm");
do_get_profile();
function run_test() {
do_check_true(OS.Constants.Path.profileDir.length > 0);
}
If I call into nsIDirectoryService e.g.
FileUtils.getDir("ProfD", [], false).path
It works.
Furthermore, if I obtain a path to the profile using nsIDirectoryService and then attempt:
OS.File.makedir(OS.Path.join(profileDir, "foobar"))
Unix error 14 during operation makeDir (Bad address)
From OS X's mkdir(2) man page:
[EFAULT] Path points outside the process's allocated address space.
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
You need to call do_get_profile() (which actually creates the profile directory) before importing osfile.jsm (which uses that information).
With this, WFM.
Assignee: nobody → dteller
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Self-reviewing this additional test.
Attachment #680307 -
Attachment is obsolete: true
Attachment #680730 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 4•12 years ago
|
||
before importing? That sounds bad: what happens if somebody imports osfile.jsm before we start the profile in a normal browser run? It should really be a lazy lookup.
Reporter | ||
Comment 5•12 years ago
|
||
Or if OS.File fails spectacularly if the profile isn't available, perhaps it should throw at import or call time. Silently not working as expected is bad behavior for an API.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> before importing? That sounds bad: what happens if somebody imports
> osfile.jsm before we start the profile in a normal browser run? It should
> really be a lazy lookup.
I do not think that I can do that. If my memory serves, |NS_GetSpecialDirectory| can only be done from the main thread, which means that it needs to be done before the thread is spawned.
Also, note that this is the exact same semantics as |NS_GetSpecialDirectory|.
(In reply to Gregory Szorc [:gps] from comment #5)
> Or if OS.File fails spectacularly if the profile isn't available, perhaps it
> should throw at import or call time. Silently not working as expected is bad
> behavior for an API.
Well, OS.File works nicely if the profile isn't available. However, OS.Constants.Path.profileDir doesn't, so if it is not getter, I could certainly turn it into a getter that throws an error.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> Well, OS.File works nicely if the profile isn't available. However,
> OS.Constants.Path.profileDir doesn't, so if it is not getter, I could
> certainly turn it into a getter that throws an error.
Of course, I meant "if it is not available".
Comment 8•12 years ago
|
||
The notification that a profile is now available is "profile-do-change". If, when the module is imported, the profile directory is not yet available, you can (should) register a listener and set it when that is fired.
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)
> The notification that a profile is now available is "profile-do-change". If,
> when the module is imported, the profile directory is not yet available, you
> can (should) register a listener and set it when that is fired.
Tell me if I'm wrong, but I seem to remember that I cannot register a listener on a non-main thread. If so, your solution will not work for workers, and at best we end up with a main thread and workers disagreeing on the value/definedness of OS.Constants.Path.profileDir
Comment 10•12 years ago
|
||
I don't understand what you mean. You're not allowed to use the directory service off the main thread either. So I'm presuming that this module is always created on the main thread, and at that point you can register the observer.
Assignee | ||
Comment 11•12 years ago
|
||
The module is created on the main thread. It copies data from the directory service to some C values destined to be read from both the main thread and worker threads. Registering the observer might help me with the main thread, but not with worker threads.
Now, if the definedness of OS.Constants.Path.profileDir matters that much, I should just raise an error if osfile.jsm is imported too early.
Assignee | ||
Comment 12•12 years ago
|
||
I could implement the following:
- when initializing constants, we fetch the profileDir if it is available;
- if the profileDir is not available, we add a listener to get it once it becomes available;
- accessing OS.Constants.Path.profileDir from the main thread before profileDir is available raises an error;
- accessing OS.Constants.Path.profileDir from the main thread once profileDir is available works normally;
- in workers spawned before the profileDir is available, accessing OS.Constants.Path.profileDir raises an error;
- in workers spawned once the profileDir is available, accessing OS.Constants.Path.profileDir works normally.
This sounds quite more complex, but feasible. Do you think this is better?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 14•12 years ago
|
||
After giving it a little more thought, I believe that throwing an error when accessing OS.Constants.Path.profileDir is rather un-javascriptish. We have tried quite hard to stick to web-style feature discovery in OS.File, so I believe that we should pursue on this avenue.
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #680730 -
Attachment is obsolete: true
Attachment #684043 -
Flags: review?(nfroyd)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #684044 -
Flags: review?(khuey)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #684045 -
Flags: review?(ted)
Assignee | ||
Updated•12 years ago
|
Attachment #684045 -
Attachment description: 3. Patching xpcshell so that it setting up the profile sends the right events → 3. Patching xpcshell so that setting up the profile sends the right events
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #684048 -
Flags: review?(nfroyd)
Comment 19•12 years ago
|
||
Comment on attachment 684043 [details] [diff] [review]
4. Testing the behavior
Review of attachment 684043 [details] [diff] [review]:
-----------------------------------------------------------------
You probably don't want to check this in as the first patch in the series. :)
Attachment #684043 -
Flags: review?(nfroyd) → review+
Comment 20•12 years ago
|
||
Comment on attachment 684048 [details] [diff] [review]
3. Patching OS.File to be able to add profileDir even if osfile.jsm is imported before the profile is set up
Review of attachment 684048 [details] [diff] [review]:
-----------------------------------------------------------------
You will rearrange this series before committing, yes?
::: toolkit/components/osfile/osfile_async_front.jsm
@@ +51,5 @@
>
> +// If osfile.jsm is imported before "profile-do-change", we do not
> +// have a profile directory yet and OS.Constants.Path.profileDir is
> +// not defined. In this case, we need to observe "profile-do-change"
> +// and set OS.Constants.Path.profileDir as soon as it becomes available.
I think the first sentence could be a lot shorter:
"If profileDir is not available, a profile has not been set up."
@@ +54,5 @@
> +// not defined. In this case, we need to observe "profile-do-change"
> +// and set OS.Constants.Path.profileDir as soon as it becomes available.
> +if (!("profileDir" in OS.Constants.Path)) {
> + Components.utils.import("resource://gre/modules/Services.jsm");
> + Components.utils.import("resource://gre/modules/Services.jsm");
Duplicate import.
Attachment #684048 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #20)
> Comment on attachment 684048 [details] [diff] [review]
> 4. Patching OS.File to be able to add profileDir even if osfile.jsm is
> imported before the profile is set up
>
> Review of attachment 684048 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> You will rearrange this series before committing, yes?
What do you mean?
>
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +51,5 @@
> >
> > +// If osfile.jsm is imported before "profile-do-change", we do not
> > +// have a profile directory yet and OS.Constants.Path.profileDir is
> > +// not defined. In this case, we need to observe "profile-do-change"
> > +// and set OS.Constants.Path.profileDir as soon as it becomes available.
>
> I think the first sentence could be a lot shorter:
>
> "If profileDir is not available, a profile has not been set up."
It doesn't really mean the same thing, does it?
Comment 22•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> (In reply to Nathan Froyd (:froydnj) from comment #20)
> > You will rearrange this series before committing, yes?
>
> What do you mean?
Well, the patches as numbered mean that you're going to add a test that breaks prior to fixing the actual bug. That's going to break bisecting (admittedly a small thing, but annoying if it happens to bite you). And committing failing tests, even as part of a patch series, is just Not Good. I think the three fixes are in an OK order.
> > > +// If osfile.jsm is imported before "profile-do-change", we do not
> > > +// have a profile directory yet and OS.Constants.Path.profileDir is
> > > +// not defined. In this case, we need to observe "profile-do-change"
> > > +// and set OS.Constants.Path.profileDir as soon as it becomes available.
> >
> > I think the first sentence could be a lot shorter:
> >
> > "If profileDir is not available, a profile has not been set up."
>
> It doesn't really mean the same thing, does it?
Which part is unclear?
Comment on attachment 684044 [details] [diff] [review]
2. Patching OSFileConstants to cope with being initialized before the profile
Review of attachment 684044 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/OSFileConstants.cpp
@@ +113,5 @@
> +
> + DelayedPathSetter() {}
> +
> + private:
> + virtual ~DelayedPathSetter(){}
There's no need for a virtual dtor here (or any explicit dtor, for that matter).
@@ +121,5 @@
> +
> +NS_IMETHODIMP
> +DelayedPathSetter::Observe(nsISupports*, const char * aTopic, const PRUnichar*)
> +{
> + if (gPaths == NULL) {
nullptr
@@ +169,5 @@
> + nsCOMPtr<nsIObserverService> obsService = do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);
> + if (NS_FAILED(rv)) {
> + return rv;
> + }
> + obsService->AddObserver(new DelayedPathSetter(), "profile-do-change", false);
This is a pretty sketchy pattern, because if the method you're calling relies on the object you're passing in being rooted bad things will happen. Better would be
nsRefPtr<DelayedPathSetter> pathSetter = new DelayedPathSetter();
objService->AddObserver(pathSetter ...)
Attachment #684044 -
Flags: review?(khuey) → review+
Updated•12 years ago
|
Attachment #684045 -
Flags: review?(ted) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #684045 -
Attachment description: 3. Patching xpcshell so that setting up the profile sends the right events → 1. Patching xpcshell so that setting up the profile sends the right events
Assignee | ||
Updated•12 years ago
|
Attachment #684048 -
Attachment description: 4. Patching OS.File to be able to add profileDir even if osfile.jsm is imported before the profile is set up → 3. Patching OS.File to be able to add profileDir even if osfile.jsm is imported before the profile is set up
Assignee | ||
Updated•12 years ago
|
Attachment #684043 -
Attachment description: 1. Testing the behavior → 4. Testing the behavior
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #23)
> Comment on attachment 684044 [details] [diff] [review]
> 2. Patching OSFileConstants to cope with being initialized before the profile
>
> Review of attachment 684044 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/system/OSFileConstants.cpp
> @@ +113,5 @@
> > +
> > + DelayedPathSetter() {}
> > +
> > + private:
> > + virtual ~DelayedPathSetter(){}
>
> There's no need for a virtual dtor here (or any explicit dtor, for that
> matter).
I must be missing something obvious, but clang insists that I need a virtual destructor because I have virtual methods.
> @@ +169,5 @@
> > + nsCOMPtr<nsIObserverService> obsService = do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);
> > + if (NS_FAILED(rv)) {
> > + return rv;
> > + }
> > + obsService->AddObserver(new DelayedPathSetter(), "profile-do-change", false);
>
> This is a pretty sketchy pattern, because if the method you're calling
> relies on the object you're passing in being rooted bad things will happen.
> Better would be
>
> nsRefPtr<DelayedPathSetter> pathSetter = new DelayedPathSetter();
> objService->AddObserver(pathSetter ...)
Good point, thanks for the catch.
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #684045 -
Attachment is obsolete: true
Attachment #687360 -
Flags: review+
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #684043 -
Attachment is obsolete: true
Attachment #687361 -
Flags: review+
Comment 27•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #24)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #23)
> > ::: dom/system/OSFileConstants.cpp
> > @@ +113,5 @@
> > > +
> > > + DelayedPathSetter() {}
> > > +
> > > + private:
> > > + virtual ~DelayedPathSetter(){}
> >
> > There's no need for a virtual dtor here (or any explicit dtor, for that
> > matter).
>
> I must be missing something obvious, but clang insists that I need a virtual
> destructor because I have virtual methods.
I think you can avoid this by declaring your class MOZ_FINAL.
Assignee | ||
Comment 28•12 years ago
|
||
nfroyd: Good point.
Attachment #684044 -
Attachment is obsolete: true
Attachment #687394 -
Flags: review+
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #684048 -
Attachment is obsolete: true
Attachment #687395 -
Flags: review+
Assignee | ||
Comment 30•12 years ago
|
||
I will wait until bug 781346 has landed to land this one, as they are bound to conflict.
Depends on: 781346
Assignee | ||
Comment 31•12 years ago
|
||
As it turns out, some tests call |do_get_profile()| more than once and if we send the event twice, we end up with assertion failures. Adding a trivial check to ensure that we only send the event once.
Attachment #687360 -
Attachment is obsolete: true
Attachment #687730 -
Flags: review?(ted)
Assignee | ||
Comment 32•12 years ago
|
||
Now that bug 781346 has landed, we need to set both profileDir and localProfileDir.
Attachment #687394 -
Attachment is obsolete: true
Attachment #687733 -
Flags: review?(khuey)
Assignee | ||
Comment 33•12 years ago
|
||
Same patch, but now we have both profileDir and localProfileDir.
Attachment #687395 -
Attachment is obsolete: true
Attachment #687734 -
Flags: review?(nfroyd)
Assignee | ||
Comment 34•12 years ago
|
||
As above, adapting to localProfileDir.
Attachment #687361 -
Attachment is obsolete: true
Attachment #687735 -
Flags: review?(nfroyd)
Updated•12 years ago
|
Attachment #687730 -
Flags: review?(ted) → review+
Updated•12 years ago
|
Attachment #687734 -
Flags: review?(nfroyd) → review+
Updated•12 years ago
|
Attachment #687735 -
Flags: review?(nfroyd) → review+
Attachment #687733 -
Flags: review?(khuey) → review+
Comment 35•12 years ago
|
||
See also Bug 820106, which is that profileDir is always "" in current trunk at browser runtime, not in xpcshell.
I'm going to test these patches, see if that makes a difference.
Comment 37•12 years ago
|
||
Reviewed code fails its own test.
0:00.45 TEST-UNEXPECTED-FAIL| /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/toolkit/components/osfile/tests/xpcshell/test_profiledir.js | true == false - See following stack:
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: do_throw :: line 452
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: _do_check_eq :: line 546
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: do_check_eq :: line 567
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: do_check_false :: line 595
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/toolkit/components/osfile/tests/xpcshell/test_profiledir.js :: run_test :: line 10
0:00.45 JS frame :: /Users/rnewman/moz/hg/services-central/testing/xpcshell/head.js :: _execute_test :: line 316
0:00.45 JS frame :: -e :: <TOP_LEVEL> :: line 1
0:00.45 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
0:00.45
Status: NEW → ASSIGNED
Comment 38•12 years ago
|
||
Oh, and it's missing licenses for some files.
Comment 39•12 years ago
|
||
profileDir isn't non-existent, it's falsy.
I changed the test to match, added a license block, and rewrote it to use add_test/run_next_test rather than the deprecated do_test_pending.
Tests pass with this change.
Attachment #687735 -
Attachment is obsolete: true
Attachment #691085 -
Flags: review?(nfroyd)
Attachment #691085 -
Flags: review?(nfroyd) → review+
Comment 40•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1982c49eca6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d0df316d44
https://hg.mozilla.org/integration/mozilla-inbound/rev/e818f0e822f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d3313397e3
Target Milestone: --- → mozilla20
Assignee | ||
Comment 41•12 years ago
|
||
That's not normal. profileDir should be non-existent.
Whiteboard: [leave open]
Assignee | ||
Comment 42•12 years ago
|
||
Please do not leave it as such. The test suite was correct. The bug is somewhere else.
Comment 43•12 years ago
|
||
Can I suggest you land the tweak in another part or a follow-up? The behavior as reviewed and landed unblocks FHR, and doesn't seem to be a regression from the previous broken behavior (e.g. bug 820106).
Assignee | ||
Comment 44•12 years ago
|
||
Ok, let's do that. I am currently investigating the issue.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 45•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c1982c49eca6
https://hg.mozilla.org/mozilla-central/rev/e9d0df316d44
https://hg.mozilla.org/mozilla-central/rev/e818f0e822f3
https://hg.mozilla.org/mozilla-central/rev/64d3313397e3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•1 year ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•