1090147 - switch tab.attach() to use the new async Worker (e10s)

Status: RESOLVED FIXED

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

Comment 1

[Tomislav Jovanovic :zombie]

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

i'm a bit sad that this requires a workaround. 

i hoped we would be able to keep a clean separation, all the content-touching/observing code in the worker-child.js, forwarding all the needed info to the parent.

unfortunately, when a tab gets closed, the message channel gets destroyed immediately, so by the time WorkerChild receives the "inner-window-destroyed" notifications, detaches, and tries to send the "detach" event to the parent, the communication fails.

the workaround is that worker-parent needs an out-of-band way to get notified, to ensure we send the "detach" event to the addon.

Comment 2

[Dave Townsend [:mossop]]

(In reply to Tomislav Jovanovic [:zombie] from comment #1)
> Created attachment 8512573 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1690
> 
> i'm a bit sad that this requires a workaround. 
> 
> i hoped we would be able to keep a clean separation, all the
> content-touching/observing code in the worker-child.js, forwarding all the
> needed info to the parent.
> 
> unfortunately, when a tab gets closed, the message channel gets destroyed
> immediately, so by the time WorkerChild receives the
> "inner-window-destroyed" notifications, detaches, and tries to send the
> "detach" event to the parent, the communication fails.

The browser message channel gets destroyed, the process message channel is still alive though. Does that help at all?

> the workaround is that worker-parent needs an out-of-band way to get
> notified, to ensure we send the "detach" event to the addon.

If I'm reading this right we rely on inner-window-destroyed being dispatched for a child process window in the parent process. I'm surprised that that happens. Am I missing something?

Comment 3

[Tomislav Jovanovic :zombie]

> The browser message channel gets destroyed, the process message channel is
> still alive though. Does that help at all?

yes, it's easy enough to use a cpmm/ppmm combo for this one thing. but that would result in two separate communication channels between worker-parent and -child, which leads to inevitable questions of refactoring (to use just the process channel), which leads to questions of performance (it's probably more optimal to use the browser channel for most of the communication), and is a whole Pandora's box that i don't feel like opening right now.


> If I'm reading this right we rely on inner-window-destroyed being dispatched
> for a child process window in the parent process. I'm surprised that that
> happens. Am I missing something?

yes, that is part of the shims implemented for (all) addons that forward some subset of observer notifications from the child to the parent process. for example, the page-mod module currently works by using those without too much changes (apart from the new async Worker).


using that shim for this is essentially punting on the whole issue described above, until we are at Stage II (removing dependance on CPOWs/shims), at which point i expect we will have a better understanding of the whole problem-space to judge the pros/cons better.


hope that sound reasonable?

Comment 4

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/24f671dc24e49e127a71de975c8956cda1435057
bug 1090147 - switch tab.attach to use async Worker

https://github.com/mozilla/addon-sdk/commit/f7171f86dcc2fa8a0adb8bda859275ac1db3f3ea
Merge pull request #1690 from zombie/1090147-tab-attach

bug 1090147 - switch tab.attach to use async Worker, r=@Mossop