OS.Constants.Path.profileDir not available in xpcshell tests

RESOLVED FIXED in mozilla20

Status

()

--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gps, Assigned: Yoric)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

3.63 KB, patch
ted
: review+
Details | Diff | Splinter Review
5.10 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.87 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.28 KB, patch
khuey
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
Blocks: 807842, 808126
Severity: normal → major
Created attachment 680307 [details] [diff] [review]
Testing profileDir with xpcshell

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
Created attachment 680730 [details] [diff] [review]
Testing profileDir with xpcshell

Self-reviewing this additional test.
Attachment #680307 - Attachment is obsolete: true
Attachment #680730 - Flags: review+
Keywords: checkin-needed

Comment 4

6 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

6 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.
Keywords: checkin-needed
(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.
(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

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

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

Comment 13

6 years ago
Yes, I do.
Flags: needinfo?(benjamin)
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.
Created attachment 684043 [details] [diff] [review]
4. Testing the behavior
Attachment #680730 - Attachment is obsolete: true
Attachment #684043 - Flags: review?(nfroyd)
Created attachment 684044 [details] [diff] [review]
2. Patching OSFileConstants to cope with being initialized before the profile
Attachment #684044 - Flags: review?(khuey)
Created attachment 684045 [details] [diff] [review]
1. Patching xpcshell so that setting up the profile sends the right events
Attachment #684045 - Flags: review?(ted)
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
Created 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
Attachment #684048 - Flags: review?(nfroyd)
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 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+
(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?
(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+
Attachment #684045 - Flags: review?(ted) → review+
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
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
Attachment #684043 - Attachment description: 1. Testing the behavior → 4. Testing the behavior
(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.
Created attachment 687360 [details] [diff] [review]
1. Patching xpcshell so that setting up the profile sends the right events
Attachment #684045 - Attachment is obsolete: true
Attachment #687360 - Flags: review+
Created attachment 687361 [details] [diff] [review]
4. Testing the behavior
Attachment #684043 - Attachment is obsolete: true
Attachment #687361 - Flags: review+
(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.
Created attachment 687394 [details] [diff] [review]
2. Patching OSFileConstants to cope with being initialized before the profile, v2

nfroyd: Good point.
Attachment #684044 - Attachment is obsolete: true
Attachment #687394 - Flags: review+
Created attachment 687395 [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, v2
Attachment #684048 - Attachment is obsolete: true
Attachment #687395 - Flags: review+
I will wait until bug 781346 has landed to land this one, as they are bound to conflict.
Depends on: 781346
Created attachment 687730 [details] [diff] [review]
1. Patching xpcshell so that setting up the profile sends the right events, v2

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)
Created attachment 687733 [details] [diff] [review]
2. Patching OSFileConstants to cope with being initialized before the profile, v3

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)
Created attachment 687734 [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, v3

Same patch, but now we have both profileDir and localProfileDir.
Attachment #687395 - Attachment is obsolete: true
Attachment #687734 - Flags: review?(nfroyd)
Created attachment 687735 [details] [diff] [review]
4. Testing the behavior, v2

As above, adapting to localProfileDir.
Attachment #687361 - Attachment is obsolete: true
Attachment #687735 - Flags: review?(nfroyd)
Attachment #687730 - Flags: review?(ted) → review+
Attachment #687734 - Flags: review?(nfroyd) → review+
Attachment #687735 - Flags: review?(nfroyd) → review+
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.
Duplicate of this bug: 820106
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
Oh, and it's missing licenses for some files.
Created attachment 691085 [details] [diff] [review]
Fixed part 4.

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)
That's not normal. profileDir should be non-existent.
Whiteboard: [leave open]
Please do not leave it as such. The test suite was correct. The bug is somewhere else.
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).
Ok, let's do that. I am currently investigating the issue.
Whiteboard: [leave open]
(Reporter)

Updated

5 years ago
Blocks: 963327
You need to log in before you can comment on or make changes to this bug.