Closed Bug 1400225 Opened 7 years ago Closed 7 years ago

System for relaying web content events to chrome

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(1 file)

Bug 1400225 - Subscribe to and relay web content DOM events.
59 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details 
We want to introduce a system that lets web content
DOM events be relayed to chrome to fix issues such as
https://bugzilla.mozilla.org/show_bug.cgi?id=1397007.

It will work in such a way that chrome, through IPC messages and a
backend service living in the content frame script, would be able to
add event listeners for any DOM event.  When the events occur they
will be sent back to the browser-specific message handler in chrome.

This will enable chrome code to wait for events to occur in content,
like this:

    await new Promise(resolve => {
      webElement.addEventListener("visibilitychange", resolve, {once: true});
      contentBrowser.minimize();
    });
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1400226
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

::: testing/marionette/dom.js:42
(Diff revision 3)
> +   *     Message manager to the current browser.
> +   */
> +  constructor(messageManager) {
> +    this.mm = messageManager;
> +    this.listeners = {};
> +    this.mm.addMessageListener("Marionette:DOM:OnEvent", this);

Do we need to worry about this potentially leaking a listener here? I am guessing not but just checking.

::: testing/marionette/dom.js:193
(Diff revision 3)
> +    for (let ev of this) {
> +      this.remove(ev);
> +    }
> +  }
> +
> +  * [Symbol.iterator]() {

I think removing the space between `*` and the `[Symbol.iterator]` would make it more obvious that its a generator. To me at least the space makes the `*` look like an error until I read the rest of the function.
Attachment #8908683 - Flags: review?(dburns) → review+
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> Do we need to worry about this potentially leaking a listener here? I am guessing not but just checking.

It is a fair point this, and the first iteration of my patch had separate start() and stop() functions for adding and removing the message listener.

However, I _think_ this should be relatively safe to do because it is attached once per content frame script, which means it will be GCed when the browser disappears.

> I think removing the space between `*` and the `[Symbol.iterator]` would make it more obvious that its a generator. To me at least the space makes the `*` look like an error until I read the rest of the function.

I agree, but eslint complained when I did "*[Symbol.iterator]".  I will file a follow-up bug to allow this for testing/marionette.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> I agree, but eslint complained when I did "*[Symbol.iterator]".  I will file a follow-up bug to allow this for testing/marionette.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1400833.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

::: testing/marionette/dom.js:66
(Diff revision 3)
> +  addEventListener(type, listener, {once = false} = {}) {
> +    if (!(type in this.listeners)) {
> +      this.listeners[type] = [];
> +    }
> +
> +    this.mm.sendAsyncMessage("Marionette:DOM:AddEventListener", {type});

We should only send this message when attaching the listener was successful, which means it should moved after line 69. What happens if line 68 fails, maybe the listener object has an own property of `once` which only allows `get`?

Also the current code doesn't take care when the listener is getting attached twice, which should not be possible.

::: testing/marionette/dom.js:90
(Diff revision 3)
> +    let stack = this.listeners[type];
> +    for (let i = 0; i < stack.length; ++i) {
> +      if (stack[i] === listener) {
> +        stack.splice(i, 1);
> +        if (stack.length == 0) {
> +          this.mm.sendAsyncMessage("Marionette:DOM:RemoveEventListener", {type});

So here we send the message only when the last listener got removed. But for registering a listener we send it each time. We should one or the other in both cases.

::: testing/marionette/dom.js:104
(Diff revision 3)
> +      return true;
> +    }
> +
> +    let stack = this.listeners[event.type];
> +    event.target = this;
> +    for (let i = 0; i < stack.length; ++i) {

Lets use `Array.forEach` in this case.

::: testing/marionette/dom.js:113
(Diff revision 3)
> +      if (listener.once) {
> +        this.removeEventListener(event.type, listener);
> +      }
> +    }
> +
> +    return !event.defaultPrevented;

Where do we handle the `cancelable` case? I think you mimiced everything similar to `EventTarget.dispatchEvent()`, so we should make sure to also do:

"The return value is false if event is cancelable..."

::: testing/marionette/dom.js:188
(Diff revision 3)
> +    this.events.delete(type);
> +  }
> +
> +  /** Ceases observing all previously registered DOM events. */
> +  clear() {
> +    for (let ev of this) {

Maybe `forEach` here and for the iterator too.

::: testing/marionette/listener.js:1916
(Diff revision 3)
>      windowUtils.updateLayerTree();
>    }
>  }
>  
> +function domAddEventListener(msg) {
> +  eventObservers.add(msg.json.type);

Please check that `msg.json` is not undefined/null. Otherwise this will produce a hang due to the JS error.

::: testing/marionette/listener.js:1920
(Diff revision 3)
> +function domAddEventListener(msg) {
> +  eventObservers.add(msg.json.type);
> +}
> +
> +function domRemoveEventListener(msg) {
> +  eventObservers.remove(msg.json.type);

Same here.

::: testing/marionette/test_dom.js:197
(Diff revision 3)
> +  equal(obs.events.size, 1);
> +  equal(win.events.length, 1);
> +
> +  obs.add("bah");
> +  equal(obs.events.size, 2);
> +  equal(win.events.length, 2);

The above lines starting with `baz` are more related to the `add` test.
Attachment #8908683 - Flags: review?(hskupin) → review-
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> It is a fair point this, and the first iteration of my patch had separate start() and stop() functions for adding and removing the message listener.
> 
> However, I _think_ this should be relatively safe to do because it is attached once per content frame script, which means it will be GCed when the browser disappears.

It's not clear to me just from reading the code where instances of WebElementEventTarget will exist. Will it be inside the Browser class of the parent process? Does it harm us when not using a weak ref as argument here? I have to claim that I don't know how the garbage collection would work across processes, but just pointing it out so we don't add something cyclic here.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> We should only send this message when attaching the listener was successful, which means it should moved after line 69. What happens if line 68 fails, maybe the listener object has an own property of `once` which only allows `get`?
> 
> Also the current code doesn't take care when the listener is getting attached twice, which should not be possible.

I’ve rearranged the order.

There is a possibility that setting "once" will overwrite something,
but to properly fix this you need a much more complicated data
structure which I don’t think is warranted.  We can deal with that
problem when and if it arises.

A listener should be allowed to be registered twice.  This is
exactly the purpose of this.listeners[type].push(listener).  It is a
dictionary of events, where an event’s listeners are appended to
a stack.  When an event message is received, all the listeners for
that event fires successively.

> So here we send the message only when the last listener got removed. But for registering a listener we send it each time. We should one or the other in both cases.

Yes, because calling Marionette:DOM:AddEventListener doesn’t
matter when you add new listeners: it’s only when you remove them
that you need to be careful not to end the subscription to events
prematurely.  Otherwise any remaining listeners for the event type
would not fire.

> Lets use `Array.forEach` in this case.

Actually we probably need to make a copy of the stack so that
removeEventListener doesn’t end up removing references from the
stack.

This is a bigger problem on :86 in the same file, which I’ve fixed
so that it iterates the stack from the top to the bottom for more
correctness.

> Where do we handle the `cancelable` case? I think you mimiced everything similar to `EventTarget.dispatchEvent()`, so we should make sure to also do:
> 
> "The return value is false if event is cancelable..."

I’m dropping this because we currently have no need to propagate
this information from DOM events.

> Maybe `forEach` here and for the iterator too.

Note that it’s using the iterator of this.events, which is a Set
here.  Unlike arrays, it is safe to delete entries on iteration.

> Please check that `msg.json` is not undefined/null. Otherwise this will produce a hang due to the JS error.

I’m not really sure what difference doing assert.object(msg.json)
or checking if "type" is present makes.  It will also throw a JS
error.  There isn’t really any good error handling or recovery
option here, so I prefer the raw JS error from trying to access a
property on an undefined value.

> The above lines starting with `baz` are more related to the `add` test.

This test is missing a call to obs.remove("bah").  The point is to
test that only one event is removed and that one is left.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> It's not clear to me just from reading the code where instances of WebElementEventTarget will exist. Will it be inside the Browser class of the parent process? Does it harm us when not using a weak ref as argument here? I have to claim that I don't know how the garbage collection would work across processes, but just pointing it out so we don't add something cyclic here.

The main point here is that we’ll not be leaking message handlers
because the browser, when discarded, will stop sending messages.
Any handlers we will have attached to its message manager will cease
to exist.

> It's not clear to me just from reading the code where instances of
> WebElementEventTarget will exist.

WebElementEventTarget is reinitialised every time the reference to
the current tab changes in browser.Context#switchToTab.

> Does it harm us when not using a weak ref as argument here?

I don’t know what you’re referring to here.

> I have to claim that I don't know how the garbage collection would
> work across processes, but just pointing it out so we don't add
> something cyclic here.

How would this be cyclic, and I’m not sure what you mean with
GC across processes?  There’s no reference to the object itself
inside WebElementEventTarget.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> I’ve rearranged the order.
> 
> There is a possibility that setting "once" will overwrite something,
> but to properly fix this you need a much more complicated data
> structure which I don’t think is warranted.  We can deal with that
> problem when and if it arises.
> 
> A listener should be allowed to be registered twice.  This is
> exactly the purpose of this.listeners[type].push(listener).  It is a
> dictionary of events, where an event’s listeners are appended to
> a stack.  When an event message is received, all the listeners for
> that event fires successively.

`listener` here referred to the exact same callback function, or object. Why would you allow to get this added multiple times? It could cause bad race conditions when the exact same events are received multiple times.

> I’m not really sure what difference doing assert.object(msg.json)
> or checking if "type" is present makes.  It will also throw a JS
> error.  There isn’t really any good error handling or recovery
> option here, so I prefer the raw JS error from trying to access a
> property on an undefined value.

As long as we don't cause a hang it will be fine.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> `listener` here referred to the exact same callback function, or object. Why would you allow to get this added multiple times? It could cause bad race conditions when the exact same events are received multiple times.

We are allowing the exact same callback function to be added
multiple times because this is how EventTarget.addEventListener
works, but one registered listener is only ever invoked once per
event.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> We are allowing the exact same callback function to be added
> multiple times because this is how EventTarget.addEventListener
> works, but one registered listener is only ever invoked once per
> event.

I don't see that you handle that in the `dispatchEvent` method. Maybe add a test to `test_WebElementEventTarget_dispatchEvent()` which tests that the exact same listener as attached twice is only called once.

> Yes, because calling Marionette:DOM:AddEventListener doesn’t
> matter when you add new listeners: it’s only when you remove them
> that you need to be careful not to end the subscription to events
> prematurely.  Otherwise any remaining listeners for the event type
> would not fire.

Not happy with that, but it won't hold off the landing.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> I don't see that you handle that in the `dispatchEvent` method. Maybe add a test to `test_WebElementEventTarget_dispatchEvent()` which tests that the exact same listener as attached twice is only called once.

As I’ve explained, it is _meant to be called multiple times_.  In
the following, you are expecting the listener to fire twice, even if
it’s the exact same listener:

> let listener = ({type}) => console.log(`${type} event fired`);
> addEventListener("foo", listener);
> addEventListener("foo", listener);

We handle this in dispatchEvent by iterating over a copy of the
stack of listeners for the event type, calling each listener
callback in succession:

> let stack = this.listeners[event.type].slice(0);
> stack.forEach(listener => {
>   listener.call(this, event);
> 
>   if (listener.once) {
>     this.removeEventListener(event.type, listener);
>   }
> });

> Not happy with that, but it won't hold off the landing.

I think there is sufficient risk that event listeners could
disappear from the content frame script in the interim between two
invocations of WebElementEventTarget.addEventListener that calling
Marionette:DOM:AddEventListener does not pose a problem.  The
ContentEventObserverService uses a set as the backing data store,
which guarantees that not more than a single DOM event listener is
present.

The eventualities I can think of off-hand are content frame script
reloads and fire-once listeners that remove the event listener.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> As I’ve explained, it is _meant to be called multiple times_.  In
> the following, you are expecting the listener to fire twice, even if
> it’s the exact same listener:
> 
> > let listener = ({type}) => console.log(`${type} event fired`);
> > addEventListener("foo", listener);
> > addEventListener("foo", listener);
> 
> We handle this in dispatchEvent by iterating over a copy of the
> stack of listeners for the event type, calling each listener
> callback in succession:
> 
> > let stack = this.listeners[event.type].slice(0);
> > stack.forEach(listener => {
> >   listener.call(this, event);
> > 
> >   if (listener.once) {
> >     this.removeEventListener(event.type, listener);
> >   }
> > });

Please note that this is not happening when you eg. pass the same method to multiple addEventListener calls for an event like `blur` in content space. It is only called once! So why are you proposing a different logic here? I think what window.addEventListener is doing here, is to not add it again, if it's already present.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> Please note that this is not happening when you eg. pass the same method to multiple addEventListener calls for an event like `blur` in content space. It is only called once! So why are you proposing a different logic here? I think what window.addEventListener is doing here, is to not add it again, if it's already present.

You are correct.  My initial test for this was wrong because I used
the visibilitychange event, which obviously fires twice: once on
iconification and once for restoration of the window.  I verified
this with https://sny.no/e/multievent.

We want to mimick the behaviour of the EventTarget.addEventListener,
so a unique listener object should, for a single event, only be
added—and be called—once.

I’ve amended the patch for this with a couple of additional tests.
Blocks: 1400256
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186318

> You are correct.  My initial test for this was wrong because I used
> the visibilitychange event, which obviously fires twice: once on
> iconification and once for restoration of the window.  I verified
> this with https://sny.no/e/multievent.
> 
> We want to mimick the behaviour of the EventTarget.addEventListener,
> so a unique listener object should, for a single event, only be
> added—and be called—once.
> 
> I’ve amended the patch for this with a couple of additional tests.

Now this looks great! Thanks for fixing that.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> The main point here is that we’ll not be leaking message handlers
> because the browser, when discarded, will stop sending messages.
> Any handlers we will have attached to its message manager will cease
> to exist.
> 
> > It's not clear to me just from reading the code where instances of
> > WebElementEventTarget will exist.
> 
> WebElementEventTarget is reinitialised every time the reference to
> the current tab changes in browser.Context#switchToTab.
> 
> > Does it harm us when not using a weak ref as argument here?
> 
> I don’t know what you’re referring to here.
> 
> > I have to claim that I don't know how the garbage collection would
> > work across processes, but just pointing it out so we don't add
> > something cyclic here.
> 
> How would this be cyclic, and I’m not sure what you mean with
> GC across processes?  There’s no reference to the object itself
> inside WebElementEventTarget.

> WebElementEventTarget is reinitialised every time the reference to
the current tab changes in browser.Context#switchToTab.

Ok, so its the parent process. But in this case what about the listeners added to the frame script instance? When we switch tabs forth and back, the registered listeners will still exist, and you setup a new one which is identical.

>> Does it harm us when not using a weak ref as argument here?
>
> I don’t know what you’re referring to here.

Well, I re-checked and basically it is about `addWeakMessageListener` vs. `addMessageListener`. We are using the former mainly in frame.js.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review186718
Attachment #8908683 - Flags: review?(hskupin) → review+
Not sure what happened with comment 22 and why it was only sent out now. Maybe have a look at it given that I totally forgot about this part right now.
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> > WebElementEventTarget is reinitialised every time the reference to
> the current tab changes in browser.Context#switchToTab.
> 
> Ok, so its the parent process. But in this case what about the listeners added to the frame script instance? When we switch tabs forth and back, the registered listeners will still exist, and you setup a new one which is identical.
> 
> >> Does it harm us when not using a weak ref as argument here?
> >
> > I don’t know what you’re referring to here.
> 
> Well, I re-checked and basically it is about `addWeakMessageListener` vs. `addMessageListener`. We are using the former mainly in frame.js.

I’m not foo familiar with when it is appropriate to use
addWeakMessageListener, but I don’t expect it is a problem for
as long as the event listener is added to WindowProxy, which will
always exist.  It might become a problem when we start allow events
subscriptions to be tied to elements?
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ead4f1cf5c5a
Subscribe to and relay web content DOM events. r=automatedtester,whimboo
Comment on attachment 8908683 [details]
Bug 1400225 - Subscribe to and relay web content DOM events.

https://reviewboard.mozilla.org/r/180324/#review185922

> I’m not foo familiar with when it is appropriate to use
> addWeakMessageListener, but I don’t expect it is a problem for
> as long as the event listener is added to WindowProxy, which will
> always exist.  It might become a problem when we start allow events
> subscriptions to be tied to elements?

I personally don't know without having to dig into this area first, which I don't have the time for. So lets keep this in mind if problems come up with leaks or hangs.
https://hg.mozilla.org/mozilla-central/rev/ead4f1cf5c5a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: