1142013 - [e10s] [dom.ipc.processCount>1] Print preview not working

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Jonathan Howard]

bp-450ea553-e444-42c0-9a9a-cf6c52150311

Print preview causes crashes every time with dom.ipc.processCount>1

[Print direct (Ctrl+P) works fine.]

Comment 1

[Tracy Walker [:tracy]]

This is now the #1 topcrash on Nightly at over 7% of all crashes. There are reports on both Windows and Linux x86 and amd64.  Crash stats has it flagged as a start up crasher.  Comments are all about Printing.  renominate for tracking-e10s

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

NI to Mike to investigate this for Thursday's triage

Comment 3

[Tracy Walker [:tracy]]

Has dropped to #2 and 3.9% of all crashes.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

My hypothesis is that what's occurring here is that we're passing a CPOW from the parent process from one content process to another, which is 100% illegal.

Looking into proving that now.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Yes, this appears to be the case.

So we're going to need to solve the problem, at some point, of making it possible for the front-end to create <xul:browser>'s and ensuring that they belong to a particular process (maybe a method on <xul:browser> itself, like createBrowser?), otherwise we're going to run into more of this as we crank the IPC process count up.

Note that the crash will be mitigated by bug 1146454. Once that lands, we won't crash, but you'd have a 1 in [value of dom.ipc.processCount] chance of having print preview just bail out on you because it can't find the right content window from the outer window ID.

Comment 6

[(Away)]

This is still a top crash on nightly and it doesn't seem right to just let it sit. To whom can I make noise?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to David Major [:dmajor] from comment #6)
> This is still a top crash on nightly and it doesn't seem right to just let
> it sit. To whom can I make noise?

dom.ipc.processCount is not a pref that is exposed in the UI, and increasing it past 1 is not currently supported. So it is not surprising that people are hitting crashes with it.

I think we might consider attempting to prevent print preview with dom.ipc.processCount > 1 as a workaround to prevent crashes, but for the most part, fiddling with dom.ipc.processCount is 100% at your own risk.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think there must be something else going on here. There are a ton of these crashes, and the uptime in most of them is only a few seconds. That doesn't sound like print preview. I filed bug 1155789 to track this as a separate issue.

Comment 9

[Jonathan Howard]

Changed from crash into exception printed in browser console since bug 1155789 landed.

Comment 10

[Jonathan Howard]

Attachment: Add preferred process selection. Use same process for print preview to make compatible when running with multiple processes

My first ever firefox patch. Bug has niggled me for long enough to want to try to fix it. WFM no idea if good approach. Alternative may be adding permenent variable to XULElement but rather get feedback first as each method has drawbacks and maybe my apprach isn't desirably. Currently bug 1165309 becomes an easy fix with this patch applied.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8709029 [details] [diff] [review]
Add preferred process selection. Use same process for print preview to make compatible when running with multiple processes

Review of attachment 8709029 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Jonathan, thanks for the patch! Very impressive for your first one for Firefox, considering the complexity of what you're diving into here.

The general idea is right - we want to, from the front end, intelligently spawn new <xul:browser>'s such that their content process is the same one that we're print previewing.

I'm not certain, however, that having the child ID be an attribute on the browser is the right choice, and I think there are a number of edge-cases that we're not considering here (what should the behaviour be if the preferred child ID doesn't map to an existing process? Is an attribute the right affordance, or should we call something on the frameloader, assuming the frameloader exists?)

We're concentrating pretty hard on getting e10s out the door with a single content process. Supporting multiple content processes is something we definitely want to do, but I don't think we're going to expend much energy on thinking about it or land patches for it until we've got the single content process out the door.

So I'm minusing this for now, but I'm going to mark it blocking the e10s-multi metabug, which you might want to follow yourself considering that you're interested in this sort of work.

Comment 12

[Gabor Krizsanits (INACTIVE)]

This test failure seem to be related to storage event, so I suspect it's related to this bug:
https://treeherder.mozilla.org/logviewer.html#?job_id=17716694&repo=try

INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug674770-1.html | The click operation worked successfully
09:27:39 INFO - startTest/<@editor/libeditor/tests/test_bug674770-1.html:42:5

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

This was fixed by bug 1165309.