Crashes at nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument*) printing very large documents

VERIFIED FIXED in Firefox 42

Status

()

Core
Printing: Output
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: smichaud, Assigned: heycam)

Tracking

({crash, regression, reproducible})

41 Branch
mozilla44
crash, regression, reproducible
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ verified, firefox43+ verified, firefox44 verified)

Details

(crash signature, URL)

Attachments

(1 attachment)

Example:

bp-944dbb40-1f33-4034-9e6f-94bee2150910

I find these crashes 100% reproducible, on both OS X and Windows, trying to print https://html.spec.whatwg.org/, which is the WHATWG HTML spec, "one-page version".  (See also https://html.spec.whatwg.org/multipage/.)  The crashes happen with e10s on or off, before any print dialog appears.

Regression range:

firefox-2015-06-16-03-02-01-mozilla-central
firefox-2015-06-17-03-02-05-mozilla-central

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ce863f9d8864&tochange=d7c148c84594
(Reporter)

Updated

2 years ago
Crash Signature: [@ nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument*) ]
(Reporter)

Comment 1

2 years ago
I'd bet one of the patches for bug 1169514 is the cause/trigger here.
Blocks: 1169514
(Reporter)

Comment 2

2 years ago
Windows example:

bp-efdcc488-5725-4748-a572-d82952150910
(Reporter)

Updated

2 years ago
Keywords: regression, reproducible
(Reporter)

Updated

2 years ago
Keywords: crash
(Reporter)

Comment 3

2 years ago
[Tracking Requested - why for this release]:

This isn't a topcrasher on either OS X or Windows.  But it *is* 100% reproducible.
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
(Reporter)

Comment 4

2 years ago
Mac non-e10s example:

bp-d3538e87-990e-4caa-8c0d-b2a4d2150910
(Reporter)

Comment 5

2 years ago
Windows non-e10s example:

bp-5fc6de0a-8605-42cf-841a-3225d2150910
:heycam, want to take a look? 
Not high volume but recent regression from 41.
tracking-firefox42: ? → +
tracking-firefox43: ? → +
Flags: needinfo?(cam)
(Assignee)

Comment 7

2 years ago
Thanks Steven and Liz; I will look at this in a week and a bit.
Assignee: nobody → cam

Comment 8

2 years ago
Simple STR: printing this SVG
https://bugzilla.mozilla.org/attachment.cgi?id=8665305

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a71f48a4e2ca3a7452d37c7aa55f9b209d3c161&tochange=7ae3e54ebb633e8895ce987a5934613303360eaa

Updated

2 years ago
Duplicate of this bug: 1207960
(Assignee)

Comment 10

2 years ago
(In reply to Loic from comment #8)
> Simple STR: printing this SVG
> https://bugzilla.mozilla.org/attachment.cgi?id=8665305
> 
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=4a71f48a4e2ca3a7452d37c7aa55f9b209d3c161&tochange=7ae3
> e54ebb633e8895ce987a5934613303360eaa

The issue here is that we're under nsIDocument::CreateStaticClone, cloning the SVG document for printing, and we call SVGDocument::EnsureNonSVGUserAgentStyleSheetsLoaded from SVGDocument::InsertChildAt.  After bug 1169514, EnsureNonSVGUserAgentStyleSheetsLoaded calls nsLayoutUtils::ShouldUseNoScriptSheet which wants to get the document's mOriginalDocument, but that is only set after the entire document cloning process has finished, back up in CreateStaticClone.
Flags: needinfo?(cam)
(Assignee)

Comment 11

2 years ago
We should be able to skip loading the on-demand non-SVG UA sheets here, since CreateStaticClone will clone the original document's loaded sheets after the document has been cloned.  There is a comment in SVGDocument::InsertChildAt about needing to do this now, but I tried printing some SVG documents (with a non-SVG root, like attachment 8665305 [details], and with an SVG root but with foreignObject HTML content in it) and it seemed to work.
(Assignee)

Comment 12

2 years ago
Created attachment 8672431 [details] [diff] [review]
patch
Attachment #8672431 - Flags: review?(jwatt)
(Assignee)

Comment 13

2 years ago
This fixes the HTML spec printing too.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
Crash Signature: [@ nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument*) ] → [@ nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument*) ] [@ nsLayoutUtils::ShouldUseNoScriptSheet ]
Cameron, do you know someone else who could review the patch? Thanks
Flags: needinfo?(cam)
Comment on attachment 8672431 [details] [diff] [review]
patch

Sorry for the delay.
Flags: needinfo?(cam)
Attachment #8672431 - Flags: review?(jwatt) → review+
Cameron, do you think this could be uplifted ? (probably in beta 9 on Thursday)
Flags: needinfo?(cam)
Keywords: checkin-needed

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61b658a2a1a
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
Comment on attachment 8672431 [details] [diff] [review]
patch

Yes I think we should uplift.

Approval Request Comment
[Feature/regressing bug #]: bug 1169514
[User impact if declined]: crashes when printing SVG documents
[Describe test coverage new/current, TreeHerder]: manually tested only; just landed on inbound
[Risks and why]: low; we just skip loading some style sheets that we know we will be loaded soon elsewhere
[String/UUID change made/needed]: N/A
Flags: needinfo?(cam)
Attachment #8672431 - Flags: approval-mozilla-beta?
Attachment #8672431 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d61b658a2a1a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
https://hg.mozilla.org/mozilla-central/rev/d61b658a2a1a
Comment on attachment 8672431 [details] [diff] [review]
patch

Fix a crash, taking it.
Should be in 42 beta 9.
Attachment #8672431 - Flags: approval-mozilla-beta?
Attachment #8672431 - Flags: approval-mozilla-beta+
Attachment #8672431 - Flags: approval-mozilla-aurora?
Attachment #8672431 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/00900b1241c4
https://hg.mozilla.org/releases/mozilla-beta/rev/c480e12985af
status-firefox41: affected → fixed
status-firefox42: affected → fixed
status-firefox41: fixed → wontfix
status-firefox43: affected → fixed
Duplicate of this bug: 1216052
Flags: qe-verify+
I was able to reproduce this issue on Firefox 44.0a1 (2015-09-25) using Windows 7 64-bit.

Verified fixed on Firefox 44.0a1 (2015-10-22/23), Firefox 43.0a2 (2015-10-22/23) and Firefox 42 Beta 9 under Windows 7 64-bit and Mac OS X 10.10.4.

This issue could not be verified on Firefox 44.0a1 with e10s enabled under Mac OS X because of https://bugzilla.mozilla.org/show_bug.cgi?id=1217190
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
Reproduced using Firefox 41.0.1.

Verified as fixed with Firefox 44 beta 8 under Win 10 (x64) and Mac OS X 10.10.5.
Also, there are no new reports in crash-stats, so marking as verified.
status-firefox44: fixed → verified
You need to log in before you can comment on or make changes to this bug.