Closed Bug 1449157 Opened 3 years ago Closed 3 years ago
Crash when clicking twice on print-preview of axis-praxis
.org variable fonts demo page
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 firstname.lastname@example.org 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.)
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)
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.
(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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c09b8a694cb5 Guard against mContent being null in nsFrame::HandlePress. r=dholbert
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.
This is pretty low frequency, but is it worth backporting to Beta anyway since it's just adding a null check?
(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 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+
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.
You need to log in before you can comment on or make changes to this bug.