1586393 - Fix BrowserTestUtils.addContentEventListener with Fission

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P2)

Description

[Andrew McCreight [:mccr8]]

This function is called in tests 7 times. 5 of those tests look like they are enabled for Fission, but 2 of them are disabled.

Comment 1

[Andrew McCreight [:mccr8]]

I haven't figured out what is going wrong yet, but I did figure out a way in which the behavior differs between Fission enabled and Fission disabled when running browser_favicon_change_not_in_document.js: with it enabled, the removeListener() method gets called almost immediately after it is registered, rather than after the test has checked how many times the event firing message was received.

It looks like the cleanup code happens when message-manager-close fires. Maybe when we load the test page right after doing the registration we end up doing a process switch, and close the message manager in that process.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1586393, part 2 - Fix BrowserTestUtils.addContentEventListener() with Fission.

Comment 3

[Andrew McCreight [:mccr8]]

The current state here is that I got something that works (I should clean up and upload the patch here) for browser_favicon_change_not_in_document.js (which is the test that currently fails in Fission that directly uses this API), but unfortunately it causes at least one of the existing users of this function that does work with Fission, browser_paste_event_at_middle_click_on_link.js, to start failing.

The way that the existing API works is that it makes every window report certain events to the parent. The parent then filters out based on the browser of whatever sent the message (:
if (msg.data == id && msg.target == browser) {
listener();
}

With my patch, the filtering is done based on the browsing context id of the browser element that is passed in. In the paste test, no windows are created with this bc id after the addContentEventListener() is called.

Now that I think about it, it looks like there's no window actor in the child being created for the current bc id, and it feels like there should be, so I'll try investigating my code that is supposed to make that happen.

Comment 4

[Andrew McCreight [:mccr8]]

Yeah, it looks like calling getActor does not actually instantiate the actor, despite the documentation to the contrary. Even if it had, my code would not actually have instantiated the actor correctly in the case of windows that existed at the time of call to addContentEventListener. Once I fixed both of those, at least this one test passes again.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1586393, part 1 - Add test-complete observer notification.

This allows generic code, like a testing JSM, to do clean up when a
test completes, in a way that will work in different test suites.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ab73cab0869
part 1 - Add test-complete observer notification. r=mconley
https://hg.mozilla.org/integration/autoland/rev/075c5b4d5827
part 2 - Fix BrowserTestUtils.addContentEventListener() with Fission. r=mconley

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9ab73cab0869
https://hg.mozilla.org/mozilla-central/rev/075c5b4d5827

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab73cab0869
https://hg.mozilla.org/integration/mozilla-inbound/rev/075c5b4d5827