1146603 - tab.attach should be made e10s safe

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Dave Townsend [:mossop]]

It uses CPOWs to do its stuff right now.

Comment 1

[James Long (:jlongster)]

Seems like this is blocking a bunch of debugger tests from running on e10s. How hard is this?

Comment 2

[Dave Townsend [:mossop]]

Not too difficult, I can give a rough outline of how someone could do it, I just have no time to do the implementation myself right now.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch

Turned out to be about as easy to write the patch as to explain it.

Here it creates a worker in the parent processes and sends a message to the child to create a child worker there. They're linked by an id in the same way as we do for workers elsewhere. Passes all tests.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8653806 [details] [diff] [review]
patch

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

::: addon-sdk/source/lib/sdk/content/utils.js
@@ +85,5 @@
>    }
>  }
>  exports.WorkerHost = WorkerHost;
> +
> +function makeChildOptions(options, id = undefined) {

is `id = undefined` necessary?

@@ +92,5 @@
> +      return [];
> +    return [String(v) for (v of [].concat(arrayOrValue))];
> +  }
> +
> +  id = id || String(uuid());

`uuid()` is always a string already

@@ +95,5 @@
> +
> +  id = id || String(uuid());
> +  return {
> +    id,
> +    contentScript: makeStringArray(options.contentScript),

I think generally we just do `[].concat(arrayOrString)`, do we need to cast the values to a string? If it's a non-string, would casting to a string even work?

::: addon-sdk/source/lib/sdk/content/worker.js
@@ +128,3 @@
>      frame = frames.getFrameForBrowser(getBrowserForTab(tab));
>  
>    function makeStringArray(arrayOrValue) {

I think this can be removed now as it was moved to makeChildOptions

Comment 5

[Dave Townsend [:mossop]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> @@ +95,5 @@
> > +
> > +  id = id || String(uuid());
> > +  return {
> > +    id,
> > +    contentScript: makeStringArray(options.contentScript),
> 
> I think generally we just do `[].concat(arrayOrString)`, do we need to cast
> the values to a string? If it's a non-string, would casting to a string even
> work?

We need to cast to strings to ensure this object is safe to send over the process boundary, you can cast anything to a string. We also need to make sure the variable isn't null or we send [null] which causes problems.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> @@ +92,5 @@
> > +      return [];
> > +    return [String(v) for (v of [].concat(arrayOrValue))];
> > +  }
> > +
> > +  id = id || String(uuid());
> 
> `uuid()` is always a string already

Actually it returns an nsIDPtr which generally casts to a string but can cause problems for the message manager when I checked last :(

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1ba1259d55e5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_dbg_event-listeners-04.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=4438935&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/4d45d62a27da

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch

In some cases we don't have the frame object when attempting to attach (it relies on an async message from content to set it up) so wait for it in that case before connecting the worker.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8656031 [details] [diff] [review]
patch

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

::: addon-sdk/source/lib/sdk/tabs/tab-firefox.js
@@ +200,5 @@
> +    });
> +
> +    let attach = frame => {
> +      let childOptions = makeChildOptions(options);
> +      frame.port.emit('sdk/tab/attach', childOptions);

nit double quotes

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e962aae1e46

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e962aae1e46

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c41767c1a9a68f52cfeb948b86f3ebb178c73f5c
Bug 1146603: Use messaging to set up the parent and child workers for tab.attach. r=jsantell