Closed Bug 1559244 Opened 2 years ago Closed 2 years ago

Write a test for crashing subframes

Categories

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

Product:
Core
Component:
DOM: Content Processes
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Project Flags:
Fission Milestone M4
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: mconley, Assigned: ablayelyfondou)

References

Details

Attachments

(3 files)

Bug 1559244 - Step 1: make BrowserTestUtils@crashBrowser function work with JSWindowActor. r?mconley
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559244 - Step 2: Add support for crashing sub-frame. r?mconley
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559244 - Step 3: Add test for crashing an oop iframe. r?mconley
47 bytes, text/x-phabricator-request
Details | Review

Bug 1559240 fixes a regression where we weren't showing the about:framecrashed error page anymore for crashing subframes. That regression went undetected for almost a month because we don't have many people testing Fission on a day-to-day basis, and because we didn't have any automated testing for this particular scenario.

So we should add some automated testing for it.

Fission Milestone: --- → ?
Priority: -- → P3

Abdoulaye, is this something you can work on?

Flags: needinfo?(abdoulayes)
Flags: needinfo?(abdoulayes) → needinfo?(ablayelyfondou)

(In reply to Neha Kochar [:neha] from comment #1)

Abdoulaye, is this something you can work on?

Sure!

Hey Mike, would you like to give some guidance here? From your comment in bug 1559240, you said some infrastructure is required to facilitate crash a subframe. Is it something that we will need to build as part of this work or is it a task to delegate to someone else.

Flags: needinfo?(ablayelyfondou) → needinfo?(mconley)
Assignee: nobody → ablayelyfondou

(In reply to Abdoulaye O. LY from comment #2)

Hey Mike, would you like to give some guidance here? From your comment in bug 1559240, you said some infrastructure is required to facilitate crash a subframe. Is it something that we will need to build as part of this work or is it a task to delegate to someone else.

Hi Abdoulaye - thanks for taking this on! I think this is something we might as well get started with in this bug.

This is the method that we use to simulate a browser tab crash:

https://searchfox.org/mozilla-central/rev/867cbb1a2b232398616e1aa42f913f37c6cb38e4/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1416-1417

I think it needs to be adapted to use JS Window Actors instead of the frame script that it's currently using. That means creating and registering some kind of BrowserTestUtils actor - maybe when loading BrowserTestUtils.jsm. That actor should be able to receive a message that causes it to basically do what the frame script does (causes a crash).

So that'd be the first step - make crashBrowser use a JS Window Actor instead of the message manager. The next step is to modify crashBrowser so that it can work with subframes. I suggest:

  1. Add an optional BrowsingContext ID argument to crashBrowser that'll crash a subframe. If not supplied, we can default to crashing the top-level frame
  2. Bonus points: rename crashBrowser to something like causeCrash or crashFrame, since the method is no longer just crashing <xul:browser>'s anymore. You'll need to update all of the callers if you do this here. We could also spin that out to a follow-up.

After that, then we need to have the method handle the case where the subframe crashes. This means using JS Window Actors to listen for when the crashing subframe is navigated to about:framecrashed (you'll need to ask the parent of the crashing frame to access the embedding element for the crashing frame, since you can no longer message a crashed frame). I'm not even sure if the minidump stuff is working for OOP iframes yet, so if it's not, we should probably file a bug to get that fixed and skip the minidump cleanup part.

Does that give you a place to get started?

Flags: needinfo?(mconley) → needinfo?(ablayelyfondou)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)

(In reply to Abdoulaye O. LY from comment #2)

Hey Mike, would you like to give some guidance here? From your comment in bug 1559240, you said some infrastructure is required to facilitate crash a subframe. Is it something that we will need to build as part of this work or is it a task to delegate to someone else.

Hi Abdoulaye - thanks for taking this on! I think this is something we might as well get started with in this bug.

This is the method that we use to simulate a browser tab crash:

https://searchfox.org/mozilla-central/rev/867cbb1a2b232398616e1aa42f913f37c6cb38e4/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1416-1417

I think it needs to be adapted to use JS Window Actors instead of the frame script that it's currently using. That means creating and registering some kind of BrowserTestUtils actor - maybe when loading BrowserTestUtils.jsm. That actor should be able to receive a message that causes it to basically do what the frame script does (causes a crash).

So that'd be the first step - make crashBrowser use a JS Window Actor instead of the message manager. The next step is to modify crashBrowser so that it can work with subframes. I suggest:

  1. Add an optional BrowsingContext ID argument to crashBrowser that'll crash a subframe. If not supplied, we can default to crashing the top-level frame
  2. Bonus points: rename crashBrowser to something like causeCrash or crashFrame, since the method is no longer just crashing <xul:browser>'s anymore. You'll need to update all of the callers if you do this here. We could also spin that out to a follow-up.

After that, then we need to have the method handle the case where the subframe crashes. This means using JS Window Actors to listen for when the crashing subframe is navigated to about:framecrashed (you'll need to ask the parent of the crashing frame to access the embedding element for the crashing frame, since you can no longer message a crashed frame). I'm not even sure if the minidump stuff is working for OOP iframes yet, so if it's not, we should probably file a bug to get that fixed and skip the minidump cleanup part.

Does that give you a place to get started?

I will probably come back to you next week but yes this is enough to get me started I think. Thank you!

Flags: needinfo?(ablayelyfondou)
Status: NEW → ASSIGNED
Fission Milestone: ? → M4
Priority: P3 → P2

Hey Abdoulaye,

Great job here so far - so the next step is actually to write a test that crashes a subframe now. Let me know if you need any guidance with that.

Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17ab0c52dca0
Step 1: make BrowserTestUtils@crashBrowser function work with JSWindowActor. r=mconley
https://hg.mozilla.org/integration/autoland/rev/467caef2ef9c
Step 2: Add support for crashing sub-frame. r=mconley
https://hg.mozilla.org/integration/autoland/rev/de76712278a6
Step 3: Add test for crashing an oop iframe. r=mconley

https://hg.mozilla.org/mozilla-central/rev/17ab0c52dca0
https://hg.mozilla.org/mozilla-central/rev/467caef2ef9c
https://hg.mozilla.org/mozilla-central/rev/de76712278a6

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1370046

Humm I noticed while working on the fullscreen patch that some intermittent failures become more frequent with JSWindowActors.

Flags: needinfo?(ablayelyfondou)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dabda4e90259
Step 1: make BrowserTestUtils@crashBrowser function work with JSWindowActor. r=mconley
https://hg.mozilla.org/integration/autoland/rev/dced8cea7b23
Step 2: Add support for crashing sub-frame. r=mconley
https://hg.mozilla.org/integration/autoland/rev/1ce7d9bbe7a1
Step 3: Add test for crashing an oop iframe. r=mconley
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18cb9c11d421
Step 1: make BrowserTestUtils@crashBrowser function work with JSWindowActor. r=mconley
https://hg.mozilla.org/integration/autoland/rev/8f3e218a7f94
Step 2: Add support for crashing sub-frame. r=mconley
https://hg.mozilla.org/integration/autoland/rev/5fd88c9ac915
Step 3: Add test for crashing an oop iframe. r=mconley
Flags: needinfo?(ablayelyfondou)

https://hg.mozilla.org/mozilla-central/rev/18cb9c11d421
https://hg.mozilla.org/mozilla-central/rev/8f3e218a7f94
https://hg.mozilla.org/mozilla-central/rev/5fd88c9ac915

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.