Closed Bug 1586393 Opened 4 months ago Closed 3 months ago

Fix BrowserTestUtils.addContentEventListener with Fission

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla72
Fission Milestone M4
Tracking Status
firefox72 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

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.

Status: NEW → ASSIGNED
Fission Milestone: --- → M4
Blocks: 1586139
Blocks: 1588193
Depends on: 1589138

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.

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.

Blocks: 1590540
Depends on: 1591447

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.

Attachment #9100034 - Attachment description: Bug 1586393 - Fix BrowserTestUtils.addContentEventListener with Fission. WIP → Bug 1586393, part 2 - Fix BrowserTestUtils.addContentEventListener() with Fission.
Pushed by amccreight@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Regressions: 1592465
Blocks: 1591873
No longer blocks: 1586139
You need to log in before you can comment on or make changes to this bug.