Closed Bug 1360350 Opened 3 years ago Closed 3 years ago

Crash in nsDeviceContext::EndDocument

Categories

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

53 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 ? wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: philipp, Assigned: jwatt)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-a9fed661-63d6-49dc-83a7-0ee310170427.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	XUL 	nsDeviceContext::EndDocument() 	gfx/src/nsDeviceContext.cpp:501
1 	XUL 	nsPrintData::~nsPrintData() 	layout/printing/nsPrintData.cpp:77
2 	XUL 	nsPrintEngine::CommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) 	layout/printing/nsPrintData.cpp:55
3 	XUL 	nsPrintEngine::Print(nsIPrintSettings*, nsIWebProgressListener*) 	layout/printing/nsPrintEngine.cpp:783
4 	XUL 	nsDocumentViewer::Print(nsIPrintSettings*, nsIWebProgressListener*) 	layout/base/nsDocumentViewer.cpp:3852
5 	XUL 	nsDocumentViewer::LoadComplete(nsresult) 	layout/base/nsDocumentViewer.cpp:1073
6 	XUL 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) 	docshell/base/nsDocShell.cpp:7603
7 	XUL 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) 	docshell/base/nsDocShell.cpp:7407
8 	XUL 	non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) 	docshell/base/nsDocShell.cpp:7304
9 	XUL 	nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) 	uriloader/base/nsDocLoader.cpp:1255
10 	XUL 	nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) 	uriloader/base/nsDocLoader.cpp:840
11 	XUL 	nsDocLoader::DocLoaderIsEmpty(bool) 	uriloader/base/nsDocLoader.cpp:730
12 	XUL 	nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	uriloader/base/nsDocLoader.cpp:612
13 	XUL 	non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	uriloader/base/nsDocLoader.cpp:468
14 	XUL 	mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) 	netwerk/base/nsLoadGroup.cpp:633
15 	XUL 	nsDocument::DoUnblockOnload() 	dom/base/nsDocument.cpp:8715
16 	XUL 	<name omitted> 	dom/events/AsyncEventDispatcher.cpp:98
17 	XUL 	<name omitted> 	xpcom/glue/nsThreadUtils.cpp:45
18 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/glue/nsCOMPtr.h:294
19 	XUL 	NS_ProcessPendingEvents(nsIThread*, unsigned int) 	xpcom/glue/nsThreadUtils.cpp:332
20 	XUL 	nsBaseAppShell::NativeEventCallback() 	widget/nsBaseAppShell.cpp:97
21 	XUL 	nsAppShell::ProcessGeckoEvents(void*) 	widget/cocoa/nsAppShell.mm:392
22 	CoreFoundation 	__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 	
23 	CoreFoundation 	__CFRunLoopDoSources0 	
24 	CoreFoundation 	__CFRunLoopRun 	
25 	CoreFoundation 	CFRunLoopRunSpecific 	
26 	HIToolbox 	RunCurrentEventLoopInMode 	
27 	HIToolbox 	ReceiveNextEventCommon 	
28 	HIToolbox 	_BlockUntilNextEventMatchingListInModeWithFilter 	
29 	AppKit 	_DPSNextEvent 	
30 	AppKit 	-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 	
31 	CoreFoundation 	-[__NSArrayM insertObject:atIndex:] 	
32 	CoreFoundation 	_CFRetain 	
33 	CoreFoundation 	CFRunLoopAddObserver 	
34 	CoreFoundation 	CFRunLoopObserverCreateWithHandler 	
35 	AppKit 	-[NSApplication setWindowsNeedUpdate:] 	
36 	AppKit 	__Block_byref_object_dispose_2624 	
37 	AppKit 	__destroy_helper_block_2620 	
38 	AppKit 	__Block_byref_object_copy_2623 	
39 	AppKit 	-[NSEvent window]

this cross-platform printing related crash is regressing in firefox 53 builds and onwards, which looks related to bug 1309272.
Flags: needinfo?(jwatt)
Keywords: regression
(In reply to [:philipp] from comment #0)
> looks related to bug 1309272.

I'm not sure what makes you think that. There's been a fair bit of churn on the printing code (and bug 1309272 isn't shipped, although some groundwork patches for it have been).
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #1)
> I'm not sure what makes you think that. There's been a fair bit of churn on
> the printing code (and bug 1309272 isn't shipped, although some groundwork
> patches for it have been).

in the bp-a9fed661-63d6-49dc-83a7-0ee310170427 report, frame 0 points to https://hg.mozilla.org/releases/mozilla-release/annotate/d345b657d381/gfx/src/nsDeviceContext.cpp#l501 which was last touched in 1309272 (and is coming from a firefox 53 installation).
sorry if the issue is unrelated to bug 1309272 after all, but as a non-developer i can only go by correlations/coincidences like these...
No, that's all fine. I was just wondering if you were seeing something that I wasn't seeing since you didn't specify your reasons. ;)
(In reply to Jonathan Watt [:jwatt] from comment #3)
> No, that's all fine. I was just wondering if you were seeing something that
> I wasn't seeing since you didn't specify your reasons. ;)

That change did remove a null check though.
Flags: needinfo?(jwatt)
Yeah. I don't understand why we would need that null check when we no longer create a separate PrintTarget for each individual page though. I'm trying to figure that out ATM but I haven't managed to repro the crash and nothing has jumped out from looking at the code.
Flags: needinfo?(jwatt)
Flags: needinfo?(jwatt)
One of the macOS crash report comments says that in the print settings the page size was set to "Other" and 0 x 0 mm for some reason, and that changing the page size to A4 fixed the crashes. The print dialog doesn't let me set the dimensions to 0 manually though...
What happens in this instance is that MakePrintTarget fails because it refuses to create a zero sized surface:

  nsDeviceContextSpecProxy::MakePrintTarget
  nsDeviceContext::InitForPrinting
  nsPrintEngine::DoCommonPrint
  nsPrintEngine::CommonPrint
  nsPrintEngine::Print
  nsDocumentViewer::Print

That failure gets propagated up to nsPrintEngine::CommonPrint where we enter the NS_FAILED(rv) block and set mPrt to nullptr, under which we crash calling nsDeviceContext::EndDocument as reported.

Ultimately I don't think the nsPrintData dtor should be calling nsDeviceContext::EndDocument if nsDeviceContext::BeginDocument has not been called.
Flags: needinfo?(jwatt)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jwatt
Attachment #8865426 - Flags: review?(bobowencode)
Attached patch patchSplinter Review
Attachment #8865426 - Attachment is obsolete: true
Attachment #8865426 - Flags: review?(bobowencode)
Attachment #8865428 - Flags: review?(bobowencode)
Attachment #8865428 - Flags: review?(bobowencode) → review+
[Tracking Requested - why for this release]: Crashes while printing are especially rage-inducing.  Asking for 53 so that it maybe can ride along in a dot-release?
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1980dae3e06b
Prevent crashes when nsPrintEngine::DoCommonPrint fails between when it sets mPrt and when it calls InitPrintDocConstruction. r=bobowen
Track 54+ as there are crashes in 54.
(In reply to Nathan Froyd [:froydnj] from comment #10)
> Crashes while printing are especially rage-inducing.

FWIW while the crash should be gone, for these users printing will still fail due to their page size being zero. I guess at least then they have a chance to search for the "An error occurred while printing" message in the error popup dialog and find the solution on:

https://support.mozilla.org/en-US/questions/1110000
https://hg.mozilla.org/mozilla-central/rev/1980dae3e06b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8865428 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1309272
[User impact if declined]: some users with broken settings will crash printing
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: manually by myself to force the error condition
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: just the patch here
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: it makes sure we only call a "finish" function if the "start" function has been called
[String changes made/needed]: none
Attachment #8865428 - Flags: approval-mozilla-beta?
Attachment #8865428 - Flags: approval-mozilla-aurora?
Bob, assuming this gets approval, would you be able to help make sure this gets landed? I'm possibly going to find it difficult to get online over the next week or so.
Flags: needinfo?(bobowencode)
(In reply to Jonathan Watt [:jwatt][back 18th May] from comment #17)
> Bob, assuming this gets approval, would you be able to help make sure this
> gets landed? I'm possibly going to find it difficult to get online over the
> next week or so.

Yes, no problem.
Flags: needinfo?(bobowencode)
Comment on attachment 8865428 [details] [diff] [review]
patch

Fix a crash. Beta54+. Should be in 54 beta 7.
Attachment #8865428 - Flags: approval-mozilla-beta?
Attachment #8865428 - Flags: approval-mozilla-beta+
Attachment #8865428 - Flags: approval-mozilla-aurora?
Attachment #8865428 - Flags: approval-mozilla-aurora-
(In reply to Jonathan Watt [:jwatt][back 18th May] from comment #16)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: manually by myself to force the
> error condition
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Jonathan's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.