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

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

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

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'd bet one of the patches for bug 1169514 is the cause/trigger here.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Windows example:

bp-efdcc488-5725-4748-a572-d82952150910

Comment 3

[Steven Michaud [:smichaud] (Retired)]

[Tracking Requested - why for this release]:

This isn't a topcrasher on either OS X or Windows.  But it *is* 100% reproducible.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Mac non-e10s example:

bp-d3538e87-990e-4caa-8c0d-b2a4d2150910

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Windows non-e10s example:

bp-5fc6de0a-8605-42cf-841a-3225d2150910

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

:heycam, want to take a look? 
Not high volume but recent regression from 41.

Comment 7

[Cameron McCormack (:heycam)]

Thanks Steven and Liz; I will look at this in a week and a bit.

Comment 8

[Loic]

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

Comment 10

[Cameron McCormack (:heycam)]

(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.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 13

[Cameron McCormack (:heycam)]

This fixes the HTML spec printing too.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Cameron, do you know someone else who could review the patch? Thanks

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8672431 [details] [diff] [review]
patch

Sorry for the delay.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Cameron, do you think this could be uplifted ? (probably in beta 9 on Thursday)

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d61b658a2a1a

Comment 18

[Cameron McCormack (:heycam)]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d61b658a2a1a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d61b658a2a1a

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8672431 [details] [diff] [review]
patch

Fix a crash, taking it.
Should be in 42 beta 9.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/00900b1241c4
https://hg.mozilla.org/releases/mozilla-beta/rev/c480e12985af

Comment 24

[Vasilica Tamas, QA [:vtamas]]

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

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering]

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.