1107609 - Task.spawn like functionality but executed on content process

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Steven MacLeod [:smacleod]]

Something that could be useful (especially for writing tests) for cutting down on boilerplate when talking interacting with content would be a Task.spawn like method, but which will execute in the content script.

The following is a js example of how I imagine this working:

    let browser = gBrowser.selectedBrowser;
    
    // spawnContent takes a browser (Or maybe a message manager instead)
    // and a generator to execute in content.
    let promise = Task.spawnContent(browser, function* content_task() {
      // We're a generator running as a task so
      // we can do async things like this.
      let something = yield somePromise();
    
      // Can touch content since we're executing inside
      // something like a frame script in the content
      // process.
      let anchors = document.querySelectorAll("a");
      for (let anchor of anchors) {
        if (anchor.href.startsWith("https://")) {
        el.classList.add("ssl");
        }
      }
    
      // Can return a value
      return "Hi, from content.";
    })
    
    promise.then((val) => {
      // val was returned by the task which was run in the
      // content process
    });

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: Patch - Implement Task.spawnContent

I was bored so a took a quick swing at writing a proof of concept for this. Patch is extremely hacky and WIP, but I thought I'd throw it up anyways to see what people thought.

I've included a modified Session Store test to show a simple use (The tests pass with this modification).

Basically I call toString on the generator / function that is passed in, message it accross to a framescript which evals the string and executes it using task.jsm. After this task finishes in the content I message the result back up and resolve the promise with it.

There are a lot of problems with this method, but I wonder if it'd be fine for use in tests only or something.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8533025 [details] [diff] [review]
Patch - Implement Task.spawnContent

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

Excellent. This is almost exactly what I had in mind.

The difference is particularly the "for use in tests" bit - I think we should use this to start the cross-browser-test module thing (bug 1093566). As a bonus, we can immediately start enforcing all the tools in that JSM are e10s-compatible.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8533025 [details] [diff] [review]
Patch - Implement Task.spawnContent

This looks awesome. eval raises my hackles a little, but I can't think of anything better off the top of my head.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

The idea is great. It's just like the runInContent() function [1] we have for sessionstore but much cleaner and available to all. Using eval() for test code is fine I think.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/head.js#520

Comment 5

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> The idea is great. It's just like the runInContent() function [1] we have
> for sessionstore but much cleaner and available to all. Using eval() for
> test code is fine I think.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/test/head.js#520

Note that we can technically workaround using eval() by just loading a framescript with a data URI. That might be necessary because of "use strict" ? Not sure if/how that propagates with frame scripts.

Comment 6

[Tom S [:evilpie]]

I would use the Function constructor and a template string. I think that would make it nicer.

Comment 7

[Steven MacLeod [:smacleod]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> I would use the Function constructor and a template string. I think that
> would make it nicer.

Could you elaborate a little? Are you saying the code to run should be passed as a (template) string rather than as an actual function?

(In reply to :Gijs Kruitbosch from comment #5)
> Note that we can technically workaround using eval() by just loading a
> framescript with a data URI. That might be necessary because of "use strict"
> ? Not sure if/how that propagates with frame scripts.

I wanted to keep this as a single framescript since there is no way to unload them and we'd start racking up memory usage we can't cleanup.

Comment 8

[Tom S [:evilpie]]

I am just talking about the content-side. (runnablestr and runnable). Sadly I don't have any good idea for not doing toString() on the parrent side.

Comment 9

[Chris Peterson [:cpeterson]]

mconley says, "this is cool. Check it out!" :)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Great idea!

It seems a bit odd to put it in Task.jsm proper, though, since it's conceptually pretty separate. Why not a separate RemoteTask.jsm with its own "spawn" method?

Comment 11

[Steven MacLeod [:smacleod]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #10)
> Great idea!
> 
> It seems a bit odd to put it in Task.jsm proper, though, since it's
> conceptually pretty separate. Why not a separate RemoteTask.jsm with its own
> "spawn" method?

Ya, I only put it in Task.jsm as a proof of concept. The next patch I throw up will have it moved :)

Comment 12

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: bz://1107609/smacleod

/r/3671 - Bug 1107609 - Implement ContentTask.spawn
/r/3673 - Bug 1107609 - Simplify test_pushstate_replacestate with ContentTask.

Pull down these commits:

hg pull review -r 4556e83b5cce17fb416591b8c104aefab01bd03c

Comment 13

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/3671/#review2905

Quibble: We've recently been flattening directory structures all over the tree. It looks like here we're making a new one which is destined to have 2 files + a moz.build. Perhaps we can use a more generic name than "ContentTask" for the directory, like, I don't know, "BrowserTestUtils" or whatever, to avoid creating lots of these dirs for utils we're intending to use widely?

::: testing/mochitest/ContentTask/ContentTask.jsm
(Diff revision 1)
> +    let deferred = Promise.defer();

Can we use PromiseUtils.defer instead?

::: testing/mochitest/ContentTask/ContentTask.jsm
(Diff revision 1)
> +let gPromises = new Map();

This makes me feel uneasy about leaks, but equally, I have no idea how to not have it. :-\

Comment 14

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/3671/#review2907

Ship It!

Comment 15

[:Gijs (he/him)]

Comment on attachment 8562248 [details]
MozReview Request: bz://1107609/smacleod

I don't understand reviewboard.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/3673/#review2955

This is great :) Should probably file a bug to get rid of runInContent() and use a ContentTask instead.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8562248 [details]
MozReview Request: bz://1107609/smacleod

(In reply to :Gijs Kruitbosch from comment #15)
> I don't understand reviewboard.

Same here :/ Thought it was going to r+ the patch for me?

Comment 18

[:Paolo Amadini]

This dynamic content script loader is pretty cool!

I already worked on similar but distinct functionality, the idea being not only making it simpler to run code in different processes, but also making it consistent across our testing frameworks.

This is how it works:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/test/loader_common.js#18

And you can find example tests in the subfolders here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/test/

I think it would be something nice to uplift to the "testing" folder, but I don't have much availability now. Unfortunately we have no strong ownership or strategy for the testing frameworks themselves, and this results in lots of duplicated effort and difficulty in making plans for it.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8562248 [details]
MozReview Request: bz://1107609/smacleod

https://reviewboard.mozilla.org/r/3669/#review2983

The code here looks solid. Just one question - see below.

::: testing/mochitest/ContentTask/ContentTask.jsm
(Diff revision 1)
> +        "chrome://mochikit/content/tests/ContentTask/content-task.js", false);

The `false` here means that we won't attempt to load the frame script into the browser if it's not yet available. Are we sure that's what we want?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/3669/#review2985

::: browser/components/sessionstore/test/browser_sessionHistory.js
(Diff revision 1)
> -  browser.messageManager.
> +  yield ContentTask.spawn(browser, {}, function* (){

Oooh - style nit - space before {

Comment 21

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/3669/#review2987

> The `false` here means that we won't attempt to load the frame script into the browser if it's not yet available. Are we sure that's what we want?

"If true, this flag means that the frame script will be loaded into any new frames opened after the loadFrameScript() call, until removeDelayedFrameScript() is called for that script."

But since I have the browser.messageManager, that would mean *only* that browser, correct? so if I set this to `true` like you're suggesting, it will just be for this browser correct?

Comment 22

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/3669/#review2997

> "If true, this flag means that the frame script will be loaded into any new frames opened after the loadFrameScript() call, until removeDelayedFrameScript() is called for that script."
> 
> But since I have the browser.messageManager, that would mean *only* that browser, correct? so if I set this to `true` like you're suggesting, it will just be for this browser correct?

As discussed in IRC, there is possibly a slim chance using `false` could break, and `true` should be safe.

Comment 23

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/rev/2483a2d08384
https://hg.mozilla.org/integration/fx-team/rev/8e3476364f4c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2483a2d08384
https://hg.mozilla.org/mozilla-central/rev/8e3476364f4c

Comment 26

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1107609 - Simplify test_pushstate_replacestate with ContentTask.

Comment 27

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1107609 - Implement ContentTask.spawn