1556076 - [10.15] Crash in [@ nsInputStreamPump::AsyncRead]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-a6c2d644-99fe-4d04-9359-a67730190531.

Seen while looking at Mac nightly crash stats: https://bit.ly/2JORAeg

So far the crashes are all on 10.15.0, and crashes are showing up for 67 and 69 so far. Some code was touched in Bug 1520868. ni on :jkt for any ideas.

Top 10 frames of crashing thread:

0 XUL nsInputStreamPump::AsyncRead netwerk/base/nsInputStreamPump.cpp:300
1 XUL nsIconChannel::AsyncOpen image/decoders/icon/mac/nsIconChannelCocoa.mm:204
2 XUL imgLoader::LoadImage image/imgLoader.cpp:2271
3 XUL nsImageBoxFrame::UpdateImage dom/base/nsContentUtils.cpp:3321
4 XUL nsImageBoxFrame::AttributeChanged layout/xul/nsImageBoxFrame.cpp:139
5 XUL mozilla::RestyleManager::AttributeChanged layout/base/RestyleManager.cpp:3408
6 XUL nsNodeUtils::AttributeChanged layout/base/PresShell.cpp:4333
7 XUL mozilla::dom::Element::SetAttrAndNotify dom/base/Element.cpp:2544
8 XUL mozilla::dom::Element::SetAttr dom/base/Element.cpp:2392
9 XUL mozilla::dom::Element::SetAttribute dom/base/Element.cpp:1397

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.

Comment 2

[Pascal Chevrel:pascalc]

There are Windows and Android crashes, to setting the Platform/OS flags to All. It's only a handful of crashes though and we had a handful as well in 66, it doesn't seem worse in 67 so this is wontfix for our current release.

Comment 3

[Marcia Knous [:marcia]]

(In reply to Pascal Chevrel:pascalc from comment #2)

There are Windows and Android crashes, to setting the Platform/OS flags to All. It's only a handful of crashes though and we had a handful as well in 66, it doesn't seem worse in 67 so this is wontfix for our current release.

97% of the crashes in this signature are in 10.15 - so I think the other 4 or so crashes on Win and Android probably aren't the same issue. Many of the comments reference crashing when downloading files. I suspect that is what the newly reported Bug 1556607 is as well.

Comment 4

[Marcia Knous [:marcia]]

Comment 3 is just the crash data in the last 7 days.

Comment 6

[berwyn.codeweaver]

Chiming in on this one, one of my coworkers updated to Catalina and experienced this. When trying to download in Safari, it's notable that it seems to trigger a systems permission prompt to allow downloads during the first download attempt after upgrading, whereas Firefox seems to segfault on download. I can't find anything in the release notes that would intimate such a change, but it might be worth digging into.

Comment 7

[nathan]

FWIW, I attempted to grant firefox full disk permissions via System Preferences, however this did not fix the issue.

Comment 8

[Marcia Knous [:marcia]]

Discussed during Channel meeting. Moving this to Downloads for now since all of the comments mention crashing while downloading.

Comment 9

[Julien Cristau [:jcristau]]

heads-up to Haik and Stephen as breakage from macos 10.15.

Comment 10

[Marcia Knous [:marcia]]

Currently this is the #5 top overall crash in the 67.0.1 release.

Comment 11

[Honza Bambas (:mayhemer)]

Adding Timothy as he was changing this in bug 1530190. I think this is an imglib bug.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Honza, that bug affects Windows only code, but the crash volume is almost all Mac. We haven't touched the Mac icon channel implementation in quite some time, so if it is somehow related to how the Mac implementation uses the stream, it would be because something else changed....

(I will investigate from the imagelib side in the meantime.)

Comment 13

[Jonathan Kingston [:jkt] he/him]

My code landed in 65 I just happen to be the last change on mercurial log.
The log looks like it crashes somewhere in AsyncRead and perhaps on the mStream code, I can't see any changes on:
nsresult rv = mStream->IsNonBlocking(&nonBlocking);
It looks to me a little like mStream isn't being set perhaps?

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

I wonder if writing to the pipe somehow failed if we got some garbage for the file size from the OS. We don't actually handle that properly. Here is a speculative fix which might help:

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eec7117f7d49409cee64b2131f573323bceedd8

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

If anybody running 10.15 is willing to try, the debug build just finished up: https://queue.taskcluster.net/v1/task/RTcyriZaTmSwTbAYHZN9WQ/runs/0/artifacts/public/build/target.dmg

If it doesn't fix the crash, please save the debug output and attach it to the bug. Hopefully some warnings got triggered to give us a hint.

Comment 16

[Marcia Knous [:marcia]]

Adding the 2 individuals who said they saw crashes. If you could please try the build in Comment 15 and report back, we would greatly appreciate it.

Comment 17

[Hayden Hong]

Running 10.15 (Build: 19A471t)
Opening about:preferences on the build above crashes without writing to wherever about:crashes writes to. Would you like the entire crash report from Console?

Apologies if it's a stupid question, I am new!

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Hayden Hong from comment #17)

Running 10.15 (Build: 19A471t)
Opening about:preferences on the build above crashes without writing to wherever about:crashes writes to. Would you like the entire crash report from Console?

Apologies if it's a stupid question, I am new!

Yes. It should have spat out a bunch of warnings to the console as well, not quite sure how stderr manifests on Mac, but that would be useful as well.

Comment 19

[Hayden Hong]

(In reply to Andrew Osmond [:aosmond] from comment #18)

Yes. It should have spat out a bunch of warnings to the console as well, not quite sure how stderr manifests on Mac, but that would be useful as well.

The output is ~1500 lines from the Console.app on macOS, much larger than I could attach in a comment here. Does that length sound right, or am I looking for logs in the wrong place? Thanks!

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Hayden Hong from comment #19)

The output is ~1500 lines from the Console.app on macOS, much larger than I
could attach in a comment here. Does that length sound right, or am I
looking for logs in the wrong place? Thanks!

That sounds right. You can attach it to this bug as a text file.

Comment 21

[Hayden Hong]

Attachment: Crashlog

Crash Log for https://queue.taskcluster.net/v1/task/RTcyriZaTmSwTbAYHZN9WQ/runs/0/artifacts/public/build/target.dmg when accessing about:preferences on 10.15 beta 19A471t.

Comment 22

[R Malone]

Also experiencing this issue when attempting to download files from any sites (gmail/facebook) using Firefox 67.0.1 on MacOS 10.15 Beta (19A471t).

In case of a small file (77 KB .jpg) the file successfully downloaded to Mac Downloads folder before the Firefox crash, but retained it's incorrect temporary name (5weWAWg5.jpg.part). When manually renamed to .jpg in Finder, the file worked fine and was complete and not corrupted.

For other files - e.g. the 83.3 MB target.dmg link in comment #15 and comment #21 above, a 1.3 MB stub was left in the Mac Downloads folder which was not usable - q0wTVAvQ.dmg.part

Likewise, when downloading a 6.2 MB pdf from gmail, a zero-size .pdf.part empty file was left in the Mac Downloads folder.

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Hayden Hong from comment #21)

Created attachment 9070112 [details]
Crashlog

Crash Log for
https://queue.taskcluster.net/v1/task/RTcyriZaTmSwTbAYHZN9WQ/runs/0/
artifacts/public/build/target.dmg when accessing about:preferences on
10.15 beta 19A471t.

Thanks for that. There is also a different type of output that can be captured if you run the build via the terminal. If you mounted the build and didn't install it in applications you would run "/Volumes/Firefox\ Nightly/Firefox\ NightlyDebug.app/Contents/MacOS/firefox" in the terminal, and then cut paste the output there. Let me know if you need help.

Comment 24

[Hayden Hong]

Attachment: stderr output

Output from running Firefox\ NightlyDebug.app/Contents/MacOS/firefox &> log.txt

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Hayden Hong from comment #24)

Created attachment 9070131 [details]
stderr output

Output from running Firefox\ NightlyDebug.app/Contents/MacOS/firefox &> log.txt

Thank you!

We are failing this line

https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/image/decoders/icon/mac/nsIconChannelCocoa.mm#276

Comment 26

[Timothy Nikkel (:tnikkel)]

And the problem is that we don't check the return value of MakeInputStream here even though it looks like we do

https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/image/decoders/icon/mac/nsIconChannelCocoa.mm#189

Confirming that, then I'll post a patch to fix that here.

We'll need a follow up to because this likely means that we won't be able to draw system icons on 10.15, but that's better than crashing.

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1556076. Restore checking the return value of MakeInputStream in nsIconChannel on mac. r?aosmond

Bug 1520868 accidentally removed this check.

Comment 28

[Timothy Nikkel (:tnikkel)]

I made the same line fail in my local build by changing the source and I got a similar looking crash, so this appears to be the issue.

Comment 29

[Timothy Nikkel (:tnikkel)]

Anyone, feel free to land this once it gets review (it'll prob be a while after it gets review before I'm able to land it myself).

Comment 30

[Masatoshi Kimura [:emk]]

Lest's annotate the method with MOZ_MUST_USE to prevent this from happening in the future.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73a4875ecd33
Restore checking the return value of MakeInputStream in nsIconChannel on mac. r=aosmond

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9070137 [details]
Bug 1556076. Restore checking the return value of MakeInputStream in nsIconChannel on mac. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: On macOS 10.15 we crash during several common operations like opening about:preferences, downloading a file, opening the downloads window.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Restores longstanding behaviour that bug 1520868 accidentally regressed of checking a return value and early returning if it is a failure. We just got lucky that we didn't fail on macOS 10.14 and below.
• String changes made/needed: none

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #26)

We'll need a follow up to because this likely means that we won't be able to
draw system icons on 10.15, but that's better than crashing.

Bug 1557218 is this followup.

Comment 34

[Timothy Nikkel (:tnikkel)]

(In reply to Masatoshi Kimura [:emk] from comment #30)

Lest's annotate the method with MOZ_MUST_USE to prevent this from
happening in the future.

Oops, just saw this.

-> bug 1557225

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 9070137 [details]
Bug 1556076. Restore checking the return value of MakeInputStream in nsIconChannel on mac. r?aosmond

macos crash fix, for 68.0b8 (hopefully)

Comment 36

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/223180843523

Comment 37

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/73a4875ecd33

Comment 38

[Pascal Chevrel:pascalc]

Comment on attachment 9070137 [details]
Bug 1556076. Restore checking the return value of MakeInputStream in nsIconChannel on mac. r?aosmond

Approved for mozilla-release and 67.0.2

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/1a95e4b8b2d2