Closed Bug 1556076 Opened 5 years ago Closed 5 years ago

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

Categories

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

Desktop
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- fixed
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: marcia, Assigned: tnikkel)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

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

Flags: needinfo?(jkt)

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

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.

OS: macOS → All
Hardware: Unspecified → All

(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 3 is just the crash data in the last 7 days.

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.

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

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

Component: DOM: Core & HTML → Downloads API
Product: Core → Toolkit
OS: All → macOS
Hardware: All → Desktop

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

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(haftandilian)
Component: Downloads API → Networking
Product: Toolkit → Core
Summary: Crash in [@ nsInputStreamPump::AsyncRead] → [10.15] Crash in [@ nsInputStreamPump::AsyncRead]
Blocks: catalina

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

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

Component: Networking → ImageLib

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.)

Flags: needinfo?(honzab.moz)
Priority: -- → P1

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?

Flags: needinfo?(jkt)

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

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.

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.

Flags: needinfo?(nathan)
Flags: needinfo?(berwyn.codeweaver)

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!

(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.

(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!

(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.

Attached file Crashlog (deleted) —

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.

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.

(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.

Attached file stderr output (deleted) —

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

(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

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.

Regressed by: 1520868

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.

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(nathan)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(haftandilian)
Flags: needinfo?(berwyn.codeweaver)

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).

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

Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/73a4875ecd33 Restore checking the return value of MakeInputStream in nsIconChannel on mac. r=aosmond
Assignee: nobody → 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
Attachment #9070137 - Flags: approval-mozilla-release?
Attachment #9070137 - Flags: approval-mozilla-beta?

(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.

(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 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)

Attachment #9070137 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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

Attachment #9070137 - Flags: approval-mozilla-release? → approval-mozilla-release+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: