1182414 - Printing a page crashes in [@ nsPresContext::HasAuthorSpecifiedRules(nsIFrame*, unsigned int) ]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Ophir LOJKINE]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150511103303

Steps to reproduce:

 - Go to http://wbo.jit.su/boards/firefox-bug
 - Hit "File > print preview"



Actual results:

Firefox crashes


Expected results:

A new window with a preview of the page should appear.

Comment 1

[Loic]

Thanks for the tescase and report.

CR FF39:
https://crash-stats.mozilla.com/report/index/c6f759b3-1d19-4679-be5a-ec2d52150710

CR FF42:
https://crash-stats.mozilla.com/report/index/56be5ba8-9b16-4c87-b60b-c6ddb2150710

Comment 2

[Loic]

Regression range:
good=2013-04-03
bad=2013-04-04
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=97cfc16ba5dc&tochange=c232bec6974d

Comment 3

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12ec336988e5&tochange=ee16c6da2c75

Triggered by:
ea4b1c3829d3	Jonathan Watt — Bug 857034 - Add support for native theming of <input type=range> on Windows. r=roc

Comment 4

[Alice0775 White]

Attachment: reduced html

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Really in this case the frames for the anonymous content should have been created at this point in the code, but I haven't been able to figure out what's going on yet.

This isn't a complete fix, but it is something that this code should be doing. We actually even paint correctly in this case because the expectation of this testcase is that we use native theming and, since none of the anonymous content's frames could be found, no style is found on them to prevent native theming.

Comment 6

[Daniel Holbert [:dholbert]]

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

Two things:

>+++ b/layout/forms/nsRangeFrame.cpp
> nsRangeFrame::ShouldUseNativeStyle() const
> {
>   return (StyleDisplay()->mAppearance == NS_THEME_RANGE) &&
>          !PresContext()->HasAuthorSpecifiedRules(const_cast<nsRangeFrame*>(this),
>                                                  (NS_AUTHOR_SPECIFIED_BORDER |
>                                                   NS_AUTHOR_SPECIFIED_BACKGROUND)) &&
>+         mTrackDiv->GetPrimaryFrame() &&
>          !PresContext()->HasAuthorSpecifiedRules(mTrackDiv->GetPrimaryFrame(),
>                                                  STYLES_DISABLING_NATIVE_THEMING) &&

 (1) We should probably cache the GetPrimaryFrame() result in a local variable instead of making two GetPrimaryFrame() calls. That's what we do in nsNumberFrame's version of this code, at least, and it seems reasonable:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsNumberControlFrame.cpp?rev=28673cc5e68b#701

 (2) Do we need this same fix in the ShouldUseNativeStyle impls in nsMeterFrame.cpp and nsProgressFrame.cpp? They also directly use the result of GetPrimaryFrame() in ShouldUseNativeStyle, so I expect they may crash in the same way:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsMeterFrame.cpp?rev=e70e937ae75b#274
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsProgressFrame.cpp?rev=e70e937ae75b#271

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: address review comments

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8649347 [details] [diff] [review]
address review comments

Looks good.

Can we regression-test this with a "reftest-print" version of the attached testcase?  That might not be close enough to actual-printing to trigger the bug; but if it is, we should include a testcase.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0c5a333d3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d73268713b2

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc0c5a333d3e
https://hg.mozilla.org/mozilla-central/rev/4d73268713b2

Comment 11

[Camelia Badau [:cbadau], Desktop QA]

I've tested on Ubuntu 12.04 64bit using Firefox 38, Firefox 39 and Firefox 42.0a1 and I cannot reproduce the initial issue (the crash) and thus I can't verify it. 

Could you please confirm that it is fixed now on Firefox 43?

Comment 12

[Ophir LOJKINE]

Yes, it's fixed now (firefox 44.0.2 on ubuntu).

Comment 13

[Camelia Badau [:cbadau], Desktop QA]

Thank you!

Marking VERIFIED FIXED based on comment 12.