Closed Bug 1107609 Opened 5 years ago Closed 5 years ago

Task.spawn like functionality but executed on content process

Categories

(Testing :: General, defect)

defect
Not set
Points:
3

Tracking

(e10s+, firefox38 fixed)

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
e10s + ---
firefox38 --- fixed

People

(Reporter: smacleod, Assigned: smacleod)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

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
    });
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.
Attachment #8533025 - Flags: feedback?(mconley)
Attachment #8533025 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8533025 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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.
Attachment #8533025 - Flags: feedback?(mconley) → feedback+
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
(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.
I would use the Function constructor and a template string. I think that would make it nicer.
(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.
Flags: needinfo?(evilpies)
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.
Flags: needinfo?(evilpies)
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
mconley says, "this is cool. Check it out!" :)
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?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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 :)
Attachment #8533025 - Attachment is obsolete: true
/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
Attachment #8562248 - Flags: review?(ttaubert)
Attachment #8562248 - Flags: review?(mconley)
Attachment #8562248 - Flags: review?(gijskruitbosch+bugs)
Points: --- → 3
Component: DOM: Content Processes → General
Flags: firefox-backlog+
Product: Core → Testing
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 on attachment 8562248 [details]
MozReview Request: bz://1107609/smacleod

I don't understand reviewboard.
Attachment #8562248 - Flags: review?(gijskruitbosch+bugs) → review+
Iteration: --- → 38.3 - 23 Feb
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 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?
Attachment #8562248 - Flags: review?(ttaubert) → review+
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 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?
Attachment #8562248 - Flags: review?(mconley) → review+
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 {
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?
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.
Blocks: 1132556
https://hg.mozilla.org/mozilla-central/rev/2483a2d08384
https://hg.mozilla.org/mozilla-central/rev/8e3476364f4c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1093954
Attachment #8562248 - Attachment is obsolete: true
Attachment #8618793 - Flags: review+
Attachment #8618794 - Flags: review+
You need to log in before you can comment on or make changes to this bug.