1058698 - need async ContentWorker for e10s

Status: RESOLVED FIXED

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

Description

[Tomislav Jovanovic :zombie]

the approach of async Sandbox from bug 1028718 is dead, no synchronous calls allowed from parent process into content sandboxes (in the child process).

this is a much cleaner, less hacky approach, to do it at a bit higher level, but wont work in a couple of cases where we still need/use the sync functionality, like context-menu and widget. 

will have to deal (deprecate/replace) with them later..

Comment 1

[Tomislav Jovanovic :zombie]

this is a work in progress, i've split the ContentWorker into a parent (public api) and a child (to be loaded in the content process) classes. 

https://github.com/zombie/addon-sdk/commit/4cce80a445601648647c0f4cc2539778bc96ce94

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Tomislav Jovanovic [:zombie] from comment #0)
...
> this is a much cleaner, less hacky approach, to do it at a bit higher level,
> but wont work in a couple of cases where we still need/use the sync
> functionality, like context-menu and widget. 

note that widget is deprecated. context-menu is the real problem, and I'm keenly interested in creative solutions. I'm not above changing the behaviour of the api tied to e10s to improve it.

Comment 3

[Dave Townsend [:mossop]]

I'd like to understand the difficulty with context-menu. I know the main Firefox code that builds the context menu will be using synchronous CPOW calls into content, I'm not sure why we can't do the same.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Dave Townsend [:mossop] from comment #3)
> I'd like to understand the difficulty with context-menu. I know the main
> Firefox code that builds the context menu will be using synchronous CPOW
> calls into content, I'm not sure why we can't do the same.

If that's the case and it works well, we can continue to provide the same api warts and all, right? That suits me as I don't think we have many spare resources just now to improve the api ( as much as I wish we did )

Comment 5

[Tomislav Jovanovic :zombie]

(In reply to Dave Townsend [:mossop] from comment #3)
> I know the main Firefox code that builds the context menu will be using 
> synchronous CPOW calls into content, I'm not sure why we can't do the same.

as far as i understand from bug 950745, and discussion with Bill McCloskey, that is fine because it's only calling from chrome code in parent into (chrome) code in child process.

what we need is a synchronous call from chrome (addon) code in parent, through chrome code in child process (that bypasses Xrays), all the way into code inside a content compartment (Sandbox in which context-menu content script is executed).

as per bug 950745 comment 12 (along with c0 and c8), this is not possible, and currently immediately crashes the child process (on purpose).

i'd love to be proven wrong on this though!

Comment 6

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

It's true that we should avoid using CPOWs to call into arbitrary content scripts. However, I don't think there's any reason that the context menu needs to be sync. We only did it that way in Firefox because a lot of existing context menu code expected to be run synchronously. However, I think the interaction could go like this:

1. contextmenu event fires in child and child send message informing the parent
2. parent synchronously fills the context menu using CPOWs (nsContextMenu.js), but also sends an async message to child asking it to run content scripts for add-ons
3. when the child replies, the parent fills in the rest of the context menu and shows it to the user

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to PTO(back 9/9) Bill McCloskey (:billm) from comment #6)
> It's true that we should avoid using CPOWs to call into arbitrary content
> scripts. However, I don't think there's any reason that the context menu
> needs to be sync. We only did it that way in Firefox because a lot of
> existing context menu code expected to be run synchronously. However, I
> think the interaction could go like this:
> 
> 1. contextmenu event fires in child and child send message informing the
> parent
> 2. parent synchronously fills the context menu using CPOWs
> (nsContextMenu.js), but also sends an async message to child asking it to
> run content scripts for add-ons
> 3. when the child replies, the parent fills in the rest of the context menu
> and shows it to the user

That seems reasonable and also horrible. I think for the SDK what I would like to explore is using this method to maintain compatibility for a while, but also exploring how to provide a simpler, async api that avoids content scripts completely. We would deprecate this current behaviour and spew warnings for a while, then shut it off. The SDK time has occasionally considered just an 'onCommand' callback defined where the callback gets some data from content - enough to then find the target node via additional means if need be.

I think there is an ( at least 80/20 ) rule for use cases:

* most% users just want to have add-on code run on command
* some add-on users want to know more from the content

In the second, rarer case I think what the developer needs is to be able to then attach a script to the page and have enough info from the handler to find the node the user right-clicked on.

Comment 8

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635

this is done (waiting for some other bugs to land), but i wanted to ask you Irakli and Dave for feedback on the usage of our Loader in the child process.

i've split the old ContentWorker code into (old) content/worker.js file that is exposed as the API to the addon, and the content/worker-child.js that is loaded in the child process.

tab-events.js listens for the "sdk/worker/create" message and instantiates the Loader using `options` passed from the parent process. that is used to load the WorkerChild class which handles the other half of the old ContentWorker functionality.


the alternative to all that is to rewrite the worker-child.js, and all the dependencies, without modules.

that would require pooling-in modules and rewriting big chunks of our code, without the ability to easily/independently test parts of the code (like we do with modules and unit-testing).

while this would not be impossible for this case, it doesn't scale well, and would be even harder when building the more complex modules that use content worker.


imho: this could be a good test case for exploring the usage of the Loader in the child process, what are good ways to do that, and even if we want to expose this functionality in some way to the addon authors (i already heard some requests).

and besides, others are already using our Loader in the child process (devtools, WebIDE, as Erik found out in bug 1037235), so we might as well benefit from the same conveniences.


one of the still open questions is how heavy our Loader really is, and if it makes sense to cache one instance of the Loader per addon (and per child process later). 

a JSM could keep track of (and a reference to) those Loader instances, while the framescript/tab-events.js would keep only the WorkerChild references alive (which maps well with the lifecycle of tabs).


what do you think?  does my long rambling (sorry!) make any sense?


(and btw, please ignore the system/xul-app.js changes in this PR, that's only a temporary workaround)

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Tomislav Jovanovic [:zombie] from comment #8)
> Created attachment 8493525 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635
> 
> this is done (waiting for some other bugs to land), but i wanted to ask you
> Irakli and Dave for feedback on the usage of our Loader in the child process.
> 
> i've split the old ContentWorker code into (old) content/worker.js file that
> is exposed as the API to the addon, and the content/worker-child.js that is
> loaded in the child process.
> 
> tab-events.js listens for the "sdk/worker/create" message and instantiates
> the Loader using `options` passed from the parent process. that is used to
> load the WorkerChild class which handles the other half of the old
> ContentWorker functionality.
> 
> 
> the alternative to all that is to rewrite the worker-child.js, and all the
> dependencies, without modules.
> 
> that would require pooling-in modules and rewriting big chunks of our code,
> without the ability to easily/independently test parts of the code (like we
> do with modules and unit-testing).
> 
> while this would not be impossible for this case, it doesn't scale well, and
> would be even harder when building the more complex modules that use content
> worker.
> 
> 
> imho: this could be a good test case for exploring the usage of the Loader
> in the child process, what are good ways to do that, and even if we want to
> expose this functionality in some way to the addon authors (i already heard
> some requests).
> 
> and besides, others are already using our Loader in the child process
> (devtools, WebIDE, as Erik found out in bug 1037235), so we might as well
> benefit from the same conveniences.
> 
> 
> one of the still open questions is how heavy our Loader really is, and if it
> makes sense to cache one instance of the Loader per addon (and per child
> process later). 
> 
> a JSM could keep track of (and a reference to) those Loader instances, while
> the framescript/tab-events.js would keep only the WorkerChild references
> alive (which maps well with the lifecycle of tabs).
> 
> 
> what do you think?  does my long rambling (sorry!) make any sense?
> 
> 
> (and btw, please ignore the system/xul-app.js changes in this PR, that's
> only a temporary workaround)

If we do decide to use loader in the content-worker (which I'd be ok with) we should definitely have one instance that all add-on's going to talk to. Two loader per add-on is an overkill. As a matter of fact in long term I would like to make all of sdk modules instances shared across all add-ons but that's far future.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

renom'ing for triage, because I think this needs to move up into m4

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8493525 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635

We're waiting on a new patch here

Comment 12

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635/files#diff-9

this is not exactly the best PR i could have submitted.. :\

not that i think the code itself is bad, just that there are probably changes that maybe don't have an obvious reason behind them, because i've been staring at the code for too long to recognize the parts that need better explanations (which i probably internalized).

so Dave, feel free to r- and/or ask for clarification as soon as you run into something that's not clear enough..

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8507463 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635/files#diff-9

This is looking really close but I'd like to do a second pass once these first round of comments have been answered/addressed.

Comment 14

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1635

addressed review comments.

this should do it..

Comment 15

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1681/files

for the record, the previous PR was written (and reviewed) as if the content/worker.js was being edited directly.

this is in fact the end goal, for this to replace the Worker (except for context-menu and widget), but it will be done in a few steps. 

so in this step, the new Worker is introduced as "content/worker-parent.js", gets its own "test-content-worker-parent.js" and page-mod is modified to use it.

Comment 16

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/fcb921a90c50e78e0710f38ee88517210a8120b5
bug 1058698 - split content/worker for e10s

and use the new one in page-mod

https://github.com/mozilla/addon-sdk/commit/9f7552109627343c8f8f4debee1a87bb588f5f5b
Merge pull request #1681 from zombie/1058698-split-worker

bug 1058698 - split content/worker for e10s, r=@Mossop