1449157 - Crash when clicking twice on print-preview of axis-praxis.org variable fonts demo page

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID:20180326220108

[Affected versions]:
Nightly 61.0a1

[Affected platforms]:
Windows 10 X64

1.Launch Nightly 61.0a1 with a new profile
2.Open https://www.axis-praxis.org/specimens/amstelvar
3.Press Ctrl+P to open print preview
4.Click twice in print preview page

[Expected result]:
Nothing should happen

[Actual result]:
Crash: https://crash-stats.mozilla.com/report/index/bb23d844-4095-4183-b969-c0b560180327

[Note]:
The crash is reproducible also with "layout.css.font-variations.enabled" on False

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] from comment #0)
> 3.Press Ctrl+P to open print preview

(On Linux, I'm using "Alt-F, V" as the key combo.)

I haven't been able to reproduce this (yet) on Linux, for what it's worth.

Roxana: I notice this page takes a little while to load. Does it matter whether you wait until the page has finished loading before triggering print-preview?  (My initial guess is that we're reconstructing frames during print-preview, due to some resource load finishing, and then we run into deleted frame pointers when we try to handle the clicks. Or something like that.)

Comment 2

[Jonathan Kew [:jfkthame]]

I've tried the steps from comment 0 several times (on Win10/x64) but have not been able to trigger a crash yet.

I'm curious about one thing:

> 3.Press Ctrl+P to open print preview

For me, Ctrl-P doesn't open Print Preview, it brings up the Print dialog. Print Preview is a separate command in the File menu, accessible (as per comment 1) with Alt-F, V. Does this indicate some difference in configuration, or is there a step I'm missing here?

UPDATE:
Aha, after writing the above, I finally succeeded in crashing it. Clicking a bunch of times in the empty space on the right-hand side of the Print Preview page seemed to be what triggered the crash.

I was also able to trigger a similar crash using Firefox 59.0.1 (and with no variation-fonts prefs enabled).

I wonder if this is not directly related to variable fonts, but perhaps has something to do with the various controls -- sliders, etc -- that are present on the axis-praxis page (but are not visible in the Print Preview)?

Here are a couple of crashes from 59.0.1, triggered in the same way:
https://crash-stats.mozilla.com/report/index/baea4737-64fd-437b-9e4a-af3db1180327
https://crash-stats.mozilla.com/report/index/172ef2ac-a267-4071-8e6c-6369b1180327

Comment 3

[Jonathan Kew [:jfkthame]]

Here's a similar Linux crash from Firefox 57.0 (distro version on Ubuntu 16.04):
https://crash-stats.mozilla.com/report/index/8183d261-6bc2-415a-800b-91aa80180327

And current Nightly on the same system:
https://crash-stats.mozilla.com/report/index/a45f02cc-dcb2-448f-a6ec-d0ee20180327

Comment 4

[roxleitan]

(In reply to Jonathan Kew (:jfkthame) from comment #2)

> I'm curious about one thing:
> 
> > 3.Press Ctrl+P to open print preview

I missed one step. Click OK after the Print dialogue is opened. (Sorry for that)

Comment 5

[Jonathan Kew [:jfkthame]]

Crash is reproducible at least as far back as 46.01a from December 2015.

Also reproducible with gfx.downloadable_fonts.enabled set to false, so that no webfonts are loaded.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Guard against mContent being null in nsFrame::HandlePress

We crash because the mouse click reaches the preview window's nsCanvasFrame, which has a null mContent pointer. So the code here should be checking whether the frame's content pointer is null before trying to call a method on the node.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Created attachment 8963131 [details] [diff] [review]
> Guard against mContent being null in nsFrame::HandlePress
> 
> We crash because the mouse click reaches the preview window's nsCanvasFrame

That sounds pretty bizarre. Do you know how/why that happens?

Patch seems reasonable, though.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Created attachment 8963131 [details] [diff] [review]
> Guard against mContent being null in nsFrame::HandlePress
> 
> We crash because the mouse click reaches the preview window's nsCanvasFrame

That sounds pretty bizarre. Do you know how/why that happens?

Patch seems reasonable, though.

Comment 9

[Daniel Holbert [:dholbert]]

(not sure how my comment 7's contents got duplicated into comment 8; sorry for that spam)

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (In reply to Jonathan Kew (:jfkthame) from comment #6)
> > Created attachment 8963131 [details] [diff] [review]
> > Guard against mContent being null in nsFrame::HandlePress
> > 
> > We crash because the mouse click reaches the preview window's nsCanvasFrame
> 
> That sounds pretty bizarre. Do you know how/why that happens?

Not at this point; it might be worth investigating that further. But in any case this patch seems like a useful and safe precaution.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09b8a694cb5
Guard against mContent being null in nsFrame::HandlePress. r=dholbert

Comment 12

[Dorel Luca [:dluca]]

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

Comment 13

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID:20180329222832

Verified fixed using the latest Nightly 61.0a1 (2018-03-29) on Windows 10 x64.

Verified with "layout.css.font-variations.enabled" set on True and also on False.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This is pretty low frequency, but is it worth backporting to Beta anyway since it's just adding a null check?

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> This is pretty low frequency, but is it worth backporting to Beta anyway
> since it's just adding a null check?

I think so. Although the crash was noticed while testing variable-fonts stuff (which is not yet preffed-on in 60), it does not actually appear to be directly related to variable fonts, and AFAIK could affect earlier releases too.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8963131 [details] [diff] [review]
Guard against mContent being null in nsFrame::HandlePress

Approval Request Comment
[Feature/Bug causing the regression]: not a regression, afaict
[User impact if declined]: possible crash when clicking in Print Preview window of certain pages (it's unclear to me if it could arise in other contexts or is somehow unique to that view)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just adds a null check, before trying to call a method on a possibly-null object
[String changes made/needed]: none

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8963131 [details] [diff] [review]
Guard against mContent being null in nsFrame::HandlePress

add a null check to prevent crash, approved for 60.0b10

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14ea71e3100e

Comment 19

[roxleitan]

I reproduced the crash mentioned in comment 2 on Beta 60.0b8 using the STR from the description.

I cannot reproduce the crash on the latest Firefox Beta 60.0b11 on Windows 10 x64.