1773099 - Crash in [@ nsPrintJob::BuildNestedPrintObjects]

Status: RESOLVED WORKSFORME

(Core :: Printing: Setup, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

5 crash reports from 3+ installations, all with Firefox 102 Betas.

Crash report: https://crash-stats.mozilla.org/report/index/b44ece5f-4bc8-478c-8a68-6756f0220607

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(doc->IsStaticDocument() || doc->IsInitialDocument())

Top 10 frames of crashing thread:

0 xul.dll nsPrintJob::BuildNestedPrintObjects layout/printing/nsPrintJob.cpp:193
1 xul.dll nsPrintJob::DoCommonPrint layout/printing/nsPrintJob.cpp:422
2 xul.dll nsPrintJob::CommonPrint layout/printing/nsPrintJob.cpp:352
3 xul.dll nsPrintJob::PrintPreview layout/printing/nsPrintJob.cpp:490
4 xul.dll nsDocumentViewer::PrintPreview layout/base/nsDocumentViewer.cpp:2982
5 xul.dll nsGlobalWindowOuter::Print dom/base/nsGlobalWindowOuter.cpp:5310
6 xul.dll mozilla::dom::BrowserChild::RecvPrintPreview dom/ipc/BrowserChild.cpp:2472
7 xul.dll mozilla::dom::PBrowserChild::OnMessageReceived ipc/ipdl/PBrowserChild.cpp:7526
8 xul.dll mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:8523
9 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:1706

Comment 1

[Daniel Holbert [:dholbert]]

Diagnostic assertions are only enabled in early-beta-and-earlier, as I recall, so this abort won't happen on release.

And if/when we do hit this code in a situation where we would-have-asserted-if-the-assertion-were-enabled, we'll just continue (skipping the document), too:

https://searchfox.org/mozilla-central/rev/c30349265c87047a324a471d2f39a216b6749262/layout/printing/nsPrintJob.cpp#188-195

RefPtr<Document> doc = docShell->GetDocument();
MOZ_DIAGNOSTIC_ASSERT(doc);
// We might find non-static documents here if the fission remoteness change
// hasn't happened / finished yet. In that case, just skip them, the same
// way we do for remote frames above.
MOZ_DIAGNOSTIC_ASSERT(doc->IsStaticDocument() || doc->IsInitialDocument());
if (!doc || !doc->IsStaticDocument()) {
  continue;

So probably/hopefully this is a non-issue for release users (other than maybe some iframe'd content being missing in their printouts, if that iframe was still loading or something?)

Looks like emilio added this diagnostic assertion in bug 1671503, so I'll add a dependency on that bug and tag him as needinfo in case he has further thoughts. Given the crash volume here, this probably isn't too concerning right now, but it's nice to be aware that we now know this diagnostic-assertion can be made to fail...

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Huh, it'd be great to know how it can fail, tbh... But lacking a test-case it's not really actionable unfortunately.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1770539

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Huh, it'd be great to know how it can fail, tbh... But lacking a test-case it's not really actionable unfortunately.

Whatever the root cause: given that we handle it gracefully, at some point we might want to consider downgrading the diagnostic assert to avoid crashing for nightly/early-beta users, if we're not able to actively/effectively investigate this at this point.

(I guess crash volume is pretty low right now, but it feels kinda-bad to be intentionally crashing without much of a reason... Though I guess some of these crashes are a sign of potential dataloss in the printed output, if we're skipping a remote iframe? In which case, maybe it's valuable to maintain some sort of signal of how-often that happens, to ensure that it's not happening super-often. It'd be nice if we could do that non-fatally.)

Comment 5

[Daniel Holbert [:dholbert]]

Calling this S3 since release users are unaffected (and even beta/nightly users have pretty low crash volume here).

Let's see if we can figure out what's going on to cause this crash, and worst-case we can eventually downgrade the assertion if we have to, per my previous comment.

Comment 6

[Daniel Holbert [:dholbert]]

Un-tagging as regression (I've moved to depend on bug 1770539 instead of being flagged as a regression from it), to avoid this showing up in carryover-regression nag lists. This bug is not super worrisome right now & not particularly nag-worthy, since it's still very low crash volume and diagnostic-only (i.e. not affecting release users), and it's semantically debatable whether this is should be considered a regression; it's just a diagnostic assert that turned out to be already failing [very very rarely] at the point that it was added, I think.

If the crash (er, diagnostic-assert-failure) volume increases, it may be worth retriaging.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 content process crashes on beta

:jwatt, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 8

[Jonathan Watt [:jwatt]]

112.0b2 clearly had a user experience this multiple times (4 installs, 15 crashes). But I just checked and there are no URLs and no useful comments in the crash data, unfortunately.

Emilio, what would you think about downgrading this MOZ_DIAGNOSTIC_ASSERT? (Taking the other side of that argument, the rate looks low enough to keep it in while 112 goes to release, and doing that might give us a better chance of getting a lead from one of our users as to what causes this and get us unblocked.)

Comment 9

[Emilio Cobos Álvarez [:emilio]]

I don't have a strong opinion, but crash volume doesn't look topcrash material to me?

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit auto_nag documentation.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.