803190 - OS.File is null in mochitest-chrome, running in tbpl

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Mihai Sucan [:msucan]]

Today I tried to use OS.File for my mochitest-chrome test, for the Web Console (in toolkit/devtools). It worked on my system really nicely.

However TBPL doesn't agree:

https://tbpl.mozilla.org/?tree=Try&rev=5c81ef93d407

TypeError: OS.File is null.

Comment 1

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

Attachment: Removing test_osfile_comms.xul scaffolding

As it turns out, the problem was due to the code of test_osfile_comms.xul. I had introduce some scaffolding in old versions to be able to test components of OS.File individually. Scaffolding cleanup was a little too aggressive.

Now that we have OS.File, replacing the scaffolding with a simple import of osfile.jsm.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 673974 [details] [diff] [review]
Removing test_osfile_comms.xul scaffolding

Review of attachment 673974 [details] [diff] [review]:
-----------------------------------------------------------------

I think this makes sense, but questions below.

::: toolkit/components/osfile/tests/mochi/test_osfile_comms.xul
@@ -29,5 @@
> -  } else {
> -    // We are under Unix
> -    Components.utils.import("resource://gre/modules/osfile/osfile_unix_allthreads.jsm");
> -    OS.File = { Error: OS.Shared.Unix.Error}; // Hack to be able to (de)serialize exceptions
> -  }

Just to double-check my own understanding here: this code wasn't importing everything that was needed to create OS.File, whereas //gre/modules/osfile.jsm does import all the necessary functionality?  Why weren't we seeing problems with this before?

@@ -52,5 @@
>          SimpleTest.ok(msg.data.condition, msg.data.description);
>          return;
>        case "finish":
> -        delete OS.File.Error; // Avoid leak
> -        OS.File = null;

Why does this need to go away?

Comment 3

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

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Just to double-check my own understanding here: this code wasn't importing
> everything that was needed to create OS.File, whereas
> //gre/modules/osfile.jsm does import all the necessary functionality?

Indeed.

> Why
> weren't we seeing problems with this before?

This was causing a side-effect visible only by mochitests. The side-effect was noticed with a new test was executed after this one.

> @@ -52,5 @@
> >          SimpleTest.ok(msg.data.condition, msg.data.description);
> >          return;
> >        case "finish":
> > -        delete OS.File.Error; // Avoid leak
> > -        OS.File = null;
> 
> Why does this need to go away?

Well, this was the side-effect. Changing the contents of a module seems bad policy.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> (In reply to Nathan Froyd (:froydnj) from comment #2)
> > Just to double-check my own understanding here: this code wasn't importing
> > everything that was needed to create OS.File, whereas
> > //gre/modules/osfile.jsm does import all the necessary functionality?
> 
> Indeed.
> 
> > Why
> > weren't we seeing problems with this before?
> 
> This was causing a side-effect visible only by mochitests. The side-effect
> was noticed with a new test was executed after this one.

"This" being the not-quite-complete import, or "this" being the effective deletion of OS.File below?  If the former, then the question stands: why was the not-quite-complete import not noticed before, since presumably we were testing all of OS.File?  If the latter, why is that necessarily a problem, as the tests should be importing OS.File anew each time, correct?

I guess I can see one hunk or the other addressing a problem, but I do not understand why both of them are needed; I'm not clear on why either one needs to be deleted at all.

> > @@ -52,5 @@
> > >          SimpleTest.ok(msg.data.condition, msg.data.description);
> > >          return;
> > >        case "finish":
> > > -        delete OS.File.Error; // Avoid leak
> > > -        OS.File = null;
> > 
> > Why does this need to go away?
> 
> Well, this was the side-effect. Changing the contents of a module seems bad
> policy.

It's not the greatest policy, but when I see things marked for "Avoid leak" getting deleted, I'd like to understand why that comment is no longer true.

Comment 5

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

(In reply to Nathan Froyd (:froydnj) from comment #4)
> "This" being the not-quite-complete import, or "this" being the effective
> deletion of OS.File below?  If the former, then the question stands: why was
> the not-quite-complete import not noticed before, since presumably we were
> testing all of OS.File?  If the latter, why is that necessarily a problem,
> as the tests should be importing OS.File anew each time, correct?

The problem was due to the deletion below. The deletion itself was necessary to avoid a leak due to the not-quite-complete import. And the not-quite-complete import was required because the module was not written yet. Now that the module is written, we can remove the hack and all its complications.

Finally, the problem was contagious to other instances of mochitest but not to other clients of OS.File running in their own modules, for reasons I can only imagine, but that might be related to compartment stuff. The problem would certainly have been noticed earlier if test_osfile_comms.xul had been executed before test_osfile_async.xul.

> I guess I can see one hunk or the other addressing a problem, but I do not
> understand why both of them are needed; I'm not clear on why either one
> needs to be deleted at all.


> > Well, this was the side-effect. Changing the contents of a module seems bad
> > policy.
> 
> It's not the greatest policy, but when I see things marked for "Avoid leak"
> getting deleted, I'd like to understand why that comment is no longer true.

Well, with a proper module instead of an object hacked together from several compartments, things are "a little" tidier. So the default module cleanup works nicely.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Now that the module is written, we can remove the hack and all its
> complications.
> 
> Finally, the problem was contagious to other instances of mochitest but not
> to other clients of OS.File running in their own modules, for reasons I can
> only imagine, but that might be related to compartment stuff. The problem
> would certainly have been noticed earlier if test_osfile_comms.xul had been
> executed before test_osfile_async.xul.

OK, I can buy some weirdness because we run all the tests in one browser instance.  r=me, let's get this in so other people won't get bitten by the same problem.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e06a3b10c0f5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e06a3b10c0f5