1146926 - sdk/content/worker.js causes unsafe CPOW usage warnings

Status: RESOLVED FIXED

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

Description

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

https://hg.mozilla.org/projects/holly/file/50e197279ad5/addon-sdk/source/lib/sdk/content/worker.js#l126

and

https://hg.mozilla.org/projects/holly/file/50e197279ad5/addon-sdk/source/lib/sdk/content/worker.js#l133

attach.define(Worker, function(worker, window) {
  // This method of attaching should be deprecated
  let model = modelFor(worker);
  if (model.attached)
    detach(worker);

  model.window = window;
  let frame = null;
  let tab = getTabForContentWindow(window.top); <-- causes unsafe CPOW usage warning
  if (tab)
    frame = frames.getFrameForBrowser(getBrowserForTab(tab));

  merge(model.options, {
    id: String(uuid()),
    window: getInnerId(window),
    url: String(window.location) <-- causes unsafe CPOW usage warning
  });

  processes.port.emit('sdk/worker/create', model.options);

  connect(worker, frame, model.options);
})

Comment 1

[Gabor Krizsanits (INACTIVE)]

Right now we have child workers, but by default if one passing in a CPOW of a window we don't create one automatically. We have to fix this for video downloadhelper. I've been looking into this and it does not seem to be all that hard, I think we should block on this one.

Comment 2

[Dave Townsend [:mossop]]

If there is a way to get the process message manager that owns a given CPOW (I'm assuming we have this recorded internally somewhere) without invoking a CPOW operation then I think this is trivially doable.

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #2)
> If there is a way to get the process message manager that owns a given CPOW
> (I'm assuming we have this recorded internally somewhere) without invoking a
> CPOW operation then I think this is trivially doable.

Well, the current version broadcasts the message so even that is not necessary I have a partially ready patch for it. But I agree that would be a much cleaner solution, actually I was about to ask you if there is a way to avoid broadcasting the message in the regular case.

A few things that I'm not sure about. In worker.js is it OK if I set model.url async with a bit of delay? Is it important to set model.frame at all? I don't see an easy way to do that without CPOW interaction (might be related to your question as well).

Bill, is there a way to get the message manager that owns a given CPOW?

Comment 4

[Gabor Krizsanits (INACTIVE)]

Attachment: content-worker should handle CPOWs. v1

The patch is a bit raw, and under-tested but seem to work. What do you think about this approach? Is setting model.frame important?

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8688900 [details] [diff] [review]
content-worker should handle CPOWs. v1

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

I think we need the frame to be set since that makes worker.tab work properly. I also think we need to defer calling connect until we have the URL. I have a plan to do all that without CPOW operations though so I'm just going to steal this.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We could add a way to map a CPOW to a process message manager (none exists now). But don't you need the tab here? I don't understand how that would work.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #6)
> We could add a way to map a CPOW to a process message manager (none exists
> now). But don't you need the tab here? I don't understand how that would
> work.

Mostly I just need to send the CPOW down to the appropriate child process so it can respond with information about the window like its URL and once there I can discover the tab without CPOW operations easily enough. I've figured out a workaround for now though, just send the CPOW to all processes and the one that gets a non-CPOW is the process it lives in!

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK :-). A quick optimization I think I suggested elsewhere is to see if there are any browsers with contentWindow == window. If the window happens to be a top-level window, then we don't need to do any messaging at all. If it's not a top-level window, then we could use your approach (or maybe just fail and suggest people use a better API, which I'm hoping exists).

Comment 9

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #8)
> OK :-). A quick optimization I think I suggested elsewhere is to see if
> there are any browsers with contentWindow == window. If the window happens
> to be a top-level window, then we don't need to do any messaging at all. If
> it's not a top-level window, then we could use your approach (or maybe just
> fail and suggest people use a better API, which I'm hoping exists).

Regardless we still need to send a message to start up the child side of the worker. We could avoid having to send back the url and tab in that case but it would mean slightly different behaviour and timing for the two cases which I think is likely to lead to confusion and unexpected bugs so I'd rather just keep everything using the same paths.

Comment 10

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1146926: Fix SDK remote tests to pass with e10s enabled. r?gabor

Bug 1146926: Fix SDK remote tests to pass with e10s enabled. r?gabor

By not waiting for the tab to finish loading we end up accidentally killing
the child process somehow and later tests that expect a child process to be
present fail.

Comment 11

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1146926: Allow sending CPOWs from parent to child processes. r?gabor

Bug 1146926: Allow sending CPOWs from parent to child processes. r?gabor

Defines an emitCPOW(event, arguments, cpows) to send a dictionary of CPOW
objects to a child process or frame. The only process that will receive the
event is the one that owns all of the CPOWs sent. The CPOW dictionary will be
appended to the arguments emitted from the port.

Also gets rid of some of the redundancy in message handling so we can more
easily change the protocol in the future.

Comment 12

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1146926: Allow attaching a worker to a CPOW window without invoking CPOW operations. r?gabor

Bug 1146926: Allow attaching a worker to a CPOW window without invoking CPOW operations. r?gabor

By sending the CPOW to the child processes the one that owns it will create the
child worker and then send back the url of the window to set up the parent
side of the worker.

There are two breaking changes here. Workers invoked in this way no longer
attach synchronously. We no longer pass the window through the attach event.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8689608 [details]
MozReview Request: Bug 1146926: Fix SDK remote tests to pass with e10s enabled. r?gabor

https://reviewboard.mozilla.org/r/25667/#review23177

Awesome, these test failures bothered me a lot.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8689610 [details]
MozReview Request: Bug 1146926: Allow sending CPOWs from parent to child processes. r?gabor

https://reviewboard.mozilla.org/r/25669/#review23179

::: addon-sdk/source/lib/sdk/remote/child.js:59
(Diff revision 1)
> +    // If any objects are CPOWs then ignore this message.
> +    if (!keys.every(name => !Cu.isCrossProcessWrapper(objects[name])))

Maybe we could briefly mention here why...

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8689611 [details]
MozReview Request: Bug 1146926: Allow attaching a worker to a CPOW window without invoking CPOW operations. r?gabor

https://reviewboard.mozilla.org/r/25671/#review23181

This looks great. A lot nicer than my WIP for sure :) Thanks!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/821c2bde78f0
https://hg.mozilla.org/integration/fx-team/rev/fe382073b07b
https://hg.mozilla.org/integration/fx-team/rev/a0db720c980e

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/821c2bde78f0
https://hg.mozilla.org/mozilla-central/rev/fe382073b07b
https://hg.mozilla.org/mozilla-central/rev/a0db720c980e