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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

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

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

Comment 2

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

TryRun: https://tbpl.mozilla.org/?tree=Try&rev=8ed1a91eaee5

Comment 3

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

Attachment: Testing profileDir with xpcshell

Self-reviewing this additional test.

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

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

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

Comment 7

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

(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

[Benjamin Smedberg]

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.

Comment 9

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

(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

[Benjamin Smedberg]

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.

Comment 11

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

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.

Comment 12

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

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?

Comment 13

[Benjamin Smedberg]

Yes, I do.

Comment 14

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

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.

Comment 15

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

Attachment: 4. Testing the behavior

Comment 16

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

Attachment: 2. Patching OSFileConstants to cope with being initialized before the profile

Comment 17

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

Attachment: 1. Patching xpcshell so that setting up the profile sends the right events

Comment 18

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

Attachment: 3. Patching OS.File to be able to add profileDir even if osfile.jsm is imported before the profile is set up

Comment 19

[Nathan Froyd [:froydnj]]

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

Comment 20

[Nathan Froyd [:froydnj]]

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.

Comment 21

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

(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

[Nathan Froyd [:froydnj]]

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

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

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

Comment 24

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

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

Comment 25

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

Attachment: 1. Patching xpcshell so that setting up the profile sends the right events

Comment 26

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

Attachment: 4. Testing the behavior

Comment 27

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #24)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 28

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

Attachment: 2. Patching OSFileConstants to cope with being initialized before the profile, v2

nfroyd: Good point.

Comment 29

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

Attachment: 3. Patching OS.File to be able to add profileDir even if osfile.jsm is imported before the profile is set up, v2

Comment 30

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

I will wait until bug 781346 has landed to land this one, as they are bound to conflict.

Comment 31

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

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

Comment 32

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

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

Comment 33

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

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

Comment 34

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

Attachment: 4. Testing the behavior, v2

As above, adapting to localProfileDir.

Comment 35

[Richard Newman [:rnewman]]

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

[Richard Newman [:rnewman]]

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

Comment 38

[Richard Newman [:rnewman]]

Oh, and it's missing licenses for some files.

Comment 39

[Richard Newman [:rnewman]]

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

Comment 40

[Richard Newman [:rnewman]]

   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

Comment 41

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

That's not normal. profileDir should be non-existent.

Comment 42

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

Please do not leave it as such. The test suite was correct. The bug is somewhere else.

Comment 43

[Richard Newman [:rnewman]]

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

Comment 44

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

Ok, let's do that. I am currently investigating the issue.

Comment 45

[Ed Morley [:emorley]]

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