1220304 - Fix DOM tests that create files to work with e10s

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

There are a number of mochitest plain tests that create files using the directory service in order to perform a test, including the following:

dom/base/test/websocket_hybi/test_send-blob.html
dom/archivereader/test/test_basic.html
dom/base/test/test_XHRSendData.html

You can't use the directory service in the child process like this, so the tests are disabled.

Comment 1

[Andrew McCreight [:mccr8]]

Andrea, do you have some existing method for doing these kinds of tests in the child process? Or do we need to figure something out?

Comment 2

[Andrea Marchesini [:baku]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Andrea, do you have some existing method for doing these kinds of tests in
> the child process? Or do we need to figure something out?

We can create a File just doing: new File([42], "something.txt");
But the problem here is that we want a file that has 'real' path. I guess it's better to figure something out for testing.
Maybe some ChromeOnly ctor of some custom file obj?

Comment 3

[Andrew McCreight [:mccr8]]

I'll take a look at these.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: SpecialPowers service for creating a file.

I implemented a new SpecialPowers API for creating files. In a content Mochitest, you call SpecialPowers.openFiles() with a description of the files you want to be created (the non-directory part of the file and the contents) and success and failure callbacks. Once the files are created, the sucess callback is called with an array of File objects. If they weren't created, the failure callback is called. There is also a SpecialPowers method to close all of the files.

Andrea, does this look like a reasonable API? (I don't particularly need feedback on all of the SpecialPowers plumbing, as I'll get some feedback on that from somebody who works on mochitest stuff.)

Also, is this an okay way to create File objects? The existing code seems to all do this:
  var fileList = document.getElementById('fileList');
  SpecialPowers.wrap(fileList).value = testFile.path;
...but it seems like maybe this is just a hacky way to create a File in a mochitest?

I've also converted the ArchiveReader tests, if you'd like to see that. Those were even easier to convert, as they already were organized around callbacks.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8685169 [details] [diff] [review]
SpecialPowers service for creating a file.

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

I would like to have these files removed when SimpleTest.finish() is executed.
Alternatively we should throw an error/exception if removeFiles() is not called.

::: dom/base/test/test_XHRSendData.html
@@ +247,5 @@
>      }
>    }
>  }
>  finally {
> +  SpecialPowers.removeFiles();

can we do this automagically when finish() is called?

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +163,5 @@
>        this._messageManager.removeMessageListener("SPPrefService", this);
>        this._messageManager.removeMessageListener("SPProcessCrashService", this);
>        this._messageManager.removeMessageListener("SPPingService", this);
>        this._messageManager.removeMessageListener("SpecialPowers.Quit", this);
> +      this._messageManager.removeMessageListener("SpecialPowers.Focus", this)

;

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Andrea Marchesini (:baku) from comment #5)
> I would like to have these files removed when SimpleTest.finish() is
> executed.

That's a good idea. I think SpecialPowers.pushPermissions() does that, so I should be able to do that.

Comment 7

[Andrew McCreight [:mccr8]]

Jed created simpleFileOpener.js which is basically the same idea, and is used in dom/html/test/test_bug143220.html and dom/html/test/test_bug1146116.html, so those would ideally also be converted to use this.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8685169 [details] [diff] [review]
SpecialPowers service for creating a file.

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

::: dom/base/test/test_XHRSendData.html
@@ +247,5 @@
>      }
>    }
>  }
>  finally {
> +  SpecialPowers.removeFiles();

SimpleTest.registerCleanupFunction may be useful here.

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +165,5 @@
>        this._messageManager.removeMessageListener("SPPingService", this);
>        this._messageManager.removeMessageListener("SpecialPowers.Quit", this);
> +      this._messageManager.removeMessageListener("SpecialPowers.Focus", this)
> +      this._messageManager.removeMessageListener("SpecialPowers.OpenFiles", this);
> +      this._messageManager.removeMessageListener("SpecialPowers.RemoveFiles", this);;

I think I found where that missing semicolon went.

@@ +282,5 @@
> +            let testFile = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +            testFile.append(request.name);
> +            if (request.data) {
> +              let outStream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
> +              outStream.init(testFile, 0x02 | 0x08 | 0x20, 0666, 0);

Nit: can this have a comment to explain that it's PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE?

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jed Davis [:jld] from comment #8)
> SimpleTest.registerCleanupFunction may be useful here.
I tried that, but SimpleTest doesn't seem to be in scope of specialpowers.js. I'll just try calling removeFiles directly in SimpleTest and hope that is okay.

> Nit: can this have a comment to explain that it's PR_WRONLY | PR_CREATE_FILE
> | PR_TRUNCATE?

Good point.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make test_XHRSendData.html use SpecialPowers.createFiles().

I'm not going to ask for review until I get bug 1223831 reviewed, in case the API changes.

There are a number of patches, but the changes are very similar and not complex.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258ece2c04d

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make ArchiveReader tests use SpecialPowers.createFiles().

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make test_send-blob.html use SpecialPowers.createFiles().

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 4a - Remove trailing whitespace from test_websocket.html.

This one can get reviewed now.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 4b - Make dom/base/test/test_websocket.html use SpecialPowers.createFiles().

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make test_XHRSendData.html use SpecialPowers.createFiles().

Fixed some spacing.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make test_send-blob.html use SpecialPowers.createFiles().

Fixed some spacing.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make test_XHRSendData.html use SpecialPowers.createFiles().

b2g and linux try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9789c598b8a4

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make ArchiveReader tests use SpecialPowers.createFiles().

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make test_send-blob.html use SpecialPowers.createFiles().

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: part 4b - Make dom/base/test/test_websocket.html use SpecialPowers.createFiles().

There are more tests to be converted. I might put them in a separate bug depending on how that goes.

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8687278 [details] [diff] [review]
part 4b - Make dom/base/test/test_websocket.html use SpecialPowers.createFiles().

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

So bad that we cannot have this test in b2g yet.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea86ea60f92
https://hg.mozilla.org/integration/mozilla-inbound/rev/e80dce92846e
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e44caf8c2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21c04934076
https://hg.mozilla.org/integration/mozilla-inbound/rev/a499b2470c33

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ea86ea60f92
https://hg.mozilla.org/mozilla-central/rev/e80dce92846e
https://hg.mozilla.org/mozilla-central/rev/86e44caf8c2a
https://hg.mozilla.org/mozilla-central/rev/b21c04934076
https://hg.mozilla.org/mozilla-central/rev/a499b2470c33