Closed Bug 1657911 Opened 4 years ago Closed 4 years ago

Crash in [@ nsDeviceContext::UnregisterPageDoneCallback]

Categories

(Core :: Printing: Output, defect, P1)

Firefox 81
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- wontfix
firefox81 --- verified

People

(Reporter: emilghitta, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [print2020_v81])

Crash Data

Attachments

(3 files)

This bug is for crash report bp-305554e6-f7b5-4073-bede-1239f0200807.

Top 10 frames of crashing thread:

0 xul.dll nsDeviceContext::UnregisterPageDoneCallback 
1 xul.dll nsPrintJob::DonePrintingPages layout/printing/nsPrintJob.cpp:2519
2 xul.dll nsPrintJob::FinishPrintPreview layout/printing/nsPrintJob.cpp:2701
3 xul.dll nsDocumentViewer::Destroy layout/base/nsDocumentViewer.cpp:1760
4 xul.dll nsDocShell::Destroy docshell/base/nsDocShell.cpp:4214
5 xul.dll nsWebBrowser::SetDocShell toolkit/components/browser/nsWebBrowser.cpp:1131
6 xul.dll nsWebBrowser::InternalDestroy toolkit/components/browser/nsWebBrowser.cpp:174
7 xul.dll nsWebBrowser::Destroy toolkit/components/browser/nsWebBrowser.cpp:855
8 xul.dll mozilla::dom::BrowserChild::DestroyWindow dom/ipc/BrowserChild.cpp:896
9 xul.dll mozilla::dom::BrowserChild::RecvDestroy dom/ipc/BrowserChild.cpp:2344

Affected Platforms:

  • Windows 10 64bit
  • macOS 10.14 (see comment 2)

Unaffected Platforms:

  • Ubuntu 18.04 64bit.

Preconditions:
Ensure that print.tab_modal.enabled is set to true.

Steps to reproduce

  1. Launch Firefox.
  2. Access the following link.
  3. Open the print preview via the "Hamburger" menu.
  4. As soon as the print preview tries to open, rapidly hit the ESC button (you may have to redo steps 3 & 4 a couple of times to hit this)

Expected Result

  • Print preview is successfully closed and Firefox is stable.

Actual Result

  • Tab Crash

Regression Range

  • I don't think that this is a regression

Additional Information

  • [Suggested Severity] This is edgy enough to not be considered as an S1 bug, but I think that S2 fits here.
Attached image printPreviewCrash.gif
Blocks: 133787

I just encountered this crash on macOS & Windows (maybe Ubuntu is affected as well..I will check on Monday) with different STR (only the step 4 is different from comment 0):

Step 4: Increment the number of copies from the "Copies" counter. At about 6 copies it will crash lead to a tab crash with this signature bp-11db3293-d232-4e5a-86a3-a66f30200808

OS: Windows 10 → All
Whiteboard: [print2020_v81]
Severity: -- → S2

jwatt, perhaps you could take a look here? [Tagging you as needinfo for now to be sure this is on someone's radar, but feel free to farm it out as-needed]

Flags: needinfo?(jwatt)
Has STR: --- → yes
Blocks: 1658287
Priority: -- → P1

When I try to reproduce the crash locally on a debug build, I got an assertion MOZ_ASSERT(mIsUAWidget) in ShadowRoot::ImportNodeAndAppendChildAt. I am trying to bisect to tell which commit causes the assertion. (I am unsure whether it's related to this crash though).

CCing Emilo (he might beat me).

Also note that, as far as I can tell, the assertion in comment 4 happens on the old print preview UI as well.

Hmm, I think that's unrelated. ShadowRoot::CloneInternalDataFrom seems it should copy its mIsUaWidget bit from aOther.

Thank you, Emilo. Filed bug 1658469 for the assertion.

See Also: → 1658469

In either cases of the STR (in comment 0 or in comment 2), nsPrintJob::DonePrintingPages gets called before printData->mPrintDC gets initialized in nsPrintJob::DoCommonPrint at line 892.

In the case of comment 0, nsPrintJob::DonePrintingPages gets called via the ShowPrintDialog call at line 820.

In the case of comment 2, nsPrintJob::DonePrintingPages gets called via the InitAsRootObject call at line 691.

Though adding a null check for mPrintDC just before using the printData->mPrintDC should fix the crash, the mPrintDC usage was introduced in this commit, and it seems it's supposed to be called only in the parent process? (with checking IsSyncPagePrinting?), so we should also check whether it's in the parent process I guess.

Also note that, even with the null check, the STR in comment 0 still causes another crash at line 864 in DoCommonPrint. At the line, printSession is nullptr since we try to get (or create) an nsIPrintSession only if mIsCreatingPrintPreview is false, but in the case, mIsCreatingPrintPreview is initially true, then it's set to false later in FinishPrintPreview().

So I suppose we should store the original mIsCreatingPrintPreview value before the nsAutoScriptBlocker scope in DoCommonPrint, then we should check the value was changed along with mIsDestroying check. I am also supposing we should also check the value change after the ShowPrintDialog call along with mPrt != printData check

I am unsure whether similar situations will happen in printing though.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED

The call sites of nsDeviceContext::RegisterPageDoneCallback are in the parent
process along with the IsSyncPagePrinting check.

I've uploaded the first part of the fix in comment 8. I am now suspecting the second part is not a right way to fix it.

I hope the first part will mitigate the QA process.

Keywords: leave-open

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

Also note that, even with the null check, the STR in comment 0 still causes another crash at line 864 in DoCommonPrint.

In this case, given that the call stack in comment 0 is including nsDocumentViewer::Destroy, and in the function we call FinishPrintPreview() then call nsPrintJob::Destroy(), so I suppose we should check mIsDestroying after the ShowPrintDialog call. That said, unfortunately I can no longer reproduce the crash at lin 864 now without the change...

I've got another assertion in the STR in comment 0. The assertion is MOZ_ASSERT(!mIsBeingDestroyed) at the top of nsDocShell::MaybeCreateInitialClientSource. This is a pretty bad signal.

The reason why the assertion happens is that in enterPrintPreview in PrintingChid.jsm we dispatch a function call to the main thread and in the function call we call initOrReusePrintPreviewViewer. In the meantime, if the print preview window gets destroyed, the docshell is not longer useful.

I am going to upload an additional fix for this case.

It's possible that the doc shell is being if the print preview window gets
closed soon after it's opened.

Hello,

I managed to encounter this crash signature with the following steps on macOS10.15.6:

  1. Launch FF.
  2. Go to https://en.wikipedia.org/wiki/Facebook
  3. Toggle between Portrait/Landscape a few times until Print Preview is no longer loading.
  4. Tab crashes. (in some instances it took around 1-2 minutes for the tab to crash)

Firefox 81.0a1 Crash Report [@ nsDeviceContext::UnregisterPageDoneCallback ]

This bug is for crash report bp-11c1aa2d-c833-469a-b930-b5bba0200813.

Top 10 frames of crashing thread:

0 XUL nsDeviceContext::UnregisterPageDoneCallback gfx/src/nsDeviceContext.cpp:713
1 XUL nsPrintJob::DonePrintingPages layout/printing/nsPrintJob.cpp:2519
2 XUL nsPrintJob::FinishPrintPreview layout/printing/nsPrintJob.cpp:2703
3 XUL nsPrintJob::MaybeResumePrintAfterResourcesLoaded layout/printing/nsPrintJob.cpp:1647
4 XUL {virtual override thunk} 
5 XUL nsDocLoader::DoFireOnStateChange uriloader/base/nsDocLoader.cpp:1331
6 XUL nsDocLoader::FireOnStateChange uriloader/base/nsDocLoader.cpp:1294
7 XUL nsDocLoader::doStopURLLoad uriloader/base/nsDocLoader.cpp:898
8 XUL nsDocLoader::OnStopRequest uriloader/base/nsDocLoader.cpp:622
9 XUL {virtual override thunk}

Thank you for another STR, Vlad!

I can reproduce it locally on a debug build, and I got the MOZ_ASSERT(!mIsBeingDestroyed) I wrote in comment 12. Now it makes me convince that the patch in comment 12 is a right fix.

Encountered another crash with this signature bp-e8ac739f-0b76-4440-9182-0051e0200813 (Different STR):

  1. Launch Firefox.
  2. Access the following link
  3. Open print preview.
  4. Click print as soon as the print preview is displayed (don't let the document load inside the print preview).

Reproduced on Windows 10 64bit & macOS 10.14

Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a34b649bd8f8
Bail out from printPreviewInitialize function if the doc shell is being destroyed during dispatching the function to the main-thread. r=jwatt
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/977b2bfa0122
Call nsDeviceContext::UnregisterPageDoneCallback only in the parent process and only if IsSyncPagePrinting. r=jwatt

Hey Emil, has the crash been stopped on the latest nightly? If so, I am going to uplift the patches to beta.

Flags: needinfo?(emil.ghitta)
Regressions: 1659300

Hey Hiro.

I wanted to give a little baking time for this issue since this has been triggered by using different STR. We have conducted another testing run today and we didn't encounter this crash signature using latest Nightly (with the steps provided in this bug & with our exploratory testing).

Flags: needinfo?(emil.ghitta)

Thank you Emil!

I am closing this bug and it turns out we don't need to uplift the fix to beta since the new print preview UI pref is enabled on EARLY_BETA_OR_EARLIER.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(jwatt)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: