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

VERIFIED FIXED in Firefox 60

Status

()

defect
--
major
VERIFIED FIXED
Last year
Last year

People

(Reporter: roxana.leitan, Assigned: jfkthame)

Tracking

({crash})

61 Branch
mozilla61
x86_64
Windows 10
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(crash signature)

Attachments

(1 attachment)

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
(In reply to roxana.leitan@softvision.ro 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.)
Flags: needinfo?(roxana.leitan)
Summary: Print preview crashes when clicking twice in the page → Crash when clicking twice on print-preview of axis-praxis.org variable fonts demo page
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
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
(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)
Flags: needinfo?(roxana.leitan)
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.
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.
Attachment #8963131 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8963131 - Flags: review?(dholbert) → review+
(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.
Keywords: crash, crashreportid
(not sure how my comment 7's contents got duplicated into comment 8; sorry for that spam)
(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.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09b8a694cb5
Guard against mContent being null in nsFrame::HandlePress. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/c09b8a694cb5
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.
Status: RESOLVED → VERIFIED
This is pretty low frequency, but is it worth backporting to Beta anyway since it's just adding a null check?
Flags: needinfo?(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.
Flags: needinfo?(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
Attachment #8963131 - Flags: approval-mozilla-beta?
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
Attachment #8963131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: roxana.leitan
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.