Comment by mtklein | Hacker News Reader

Comment by mtklein | Hacker News Reader

I wrote a lot of the Skia color management code involved here, and I think maybe I can clear some things up? I'd be happy to answer any questions you might have, either here or on https://bugs.chromium.org/p/skia/issues/detail?id=10301 where we've been trying to think about how much Skia's involved with the root cause of this bug.

There's no "Skia color profile" per se. When Skia's asked to write a JPEG from a pixel buffer tagged with a color profile (SkColorSpace), we auto-generate an ICC profile programatically from the parameters that describe the color gamut and transfer functions we use to represent those color profiles. That's what the code in this file is up to: https://source.chromium.org/chromium/chromium/src/+/master:t...

So this profile is one of infinite possible profiles that'll have "Google/Skia/..big long MD5.." somewhere in them.

This profile in particular looks like it's probably describing the ProPhoto color space, which is very, very large and uses imaginary colors outside visible light to increase its gamut. While the JPEG itself is always storing values in range in the ProPhoto gamut, when you convert that to another gamut (say, to match the display, or to sRGB) it's very easy to end up with logical color channel values less than zero or greater than one, which do make a kind of sense, but not to code that expects colors are always sRGB bytes.

It's unclear to me what gamut the colors being read in that Java code are in, and that'd very much affect what the appropriate values for that LUMINOSITY_MATRIX should be. Those are the correct values to get the luminosity for sRGB colors (though I think what's being calculated here is subtly-different gamma-encoded "luma" instead), but they're not likely a sound way to get luminosity if the image is in any different gamut. A typical strategy is to convert any image you load to the display gamut early, since you'll need to do that eventually to display it. And it's more and more common that phone display gamuts are not sRGB, usually something wider now like Display P3.

From the Skia bug report:

> out of bounds write in Java

Isn't Java supposed to be a language safe from these kinds of bugs? At the very worst it throws an exception, which should be caught and dealt with accordingly… How can that crash the whole phone?

> At the very worst it throws an exception

Yes, from what I have read that's exactly what was happening, it was throwing an IndexOutOfBoundsException. Apparently, nothing was catching that exception, so the whole process was terminated.

> How can that crash the whole phone?

The main problem is that the process which was crashing is the Android equivalent of the window manager or the X server. It recovered from that crash either by restarting the process (which immediately crashed again), or by rebooting the phone (and immediately crashing again).

> Apparently, nothing was catching that exception, so the whole process was terminated.

Apposite moment perhaps to refer to the mind-boggling finding in "Simple Testing Can Prevent Most Critical Failures: An Analysis of Production Failures in Distributed Data-Intensive Systems" (Yuan et al, Usenix 2014 https://www.usenix.org/conference/osdi14/technical-sessions/...):

"almost all (92%) of the catastrophic system failures [in the real-world study] are the result of incorrect handling of non-fatal errors explicitly signaled in software"

I lends this leads credence to the go way. Verbose because than its hard to mess up

I mean... not really. This is the equivalent of `if err != nil { return nil, err }` which is so common it's a mindless default behavior worthy of memes: https://pbs.twimg.com/media/DCIF7-2W0AEAv9c.jpg

It absolutely results in the same kinds of failures.

Saw one case where it was `if err != nil { return nil }`, so the error was completely ignored and function exited without error...

Yep, I think that's exactly what happened, an uncaught java.lang.ArrayIndexOutOfBoundsException crashed the rather important com.android.systemui process.

> which should be caught and dealt with accordingly

Which is easy when you have an Either<L, R> construct for your returns or when you have checked Exceptions, which Java devs dislike.

So you get errors like this, because devs are often not aware or misremember all the failure modes of calls they make. Because they're only humans and they're using tools and practices that like to pretend everything that matters is the happy path.

Yeah, from a static safety perspective, Java’s checked exceptions are a big missed opportunity: if Java could have figured out a better syntax for them that was less annoying to deal with they could have been really interesting. As it is, they actively prevent certain sorts of abstractions.

> they actively prevent certain sorts of abstractions.

Like what?

It’s been a while, but every time I’ve tried to mix higher order functions and checked exceptions in Java, I’ve found it pretty painful: there’s some annoying type checking issue around the way lambdas work in Stream.map or similar. Maybe I’ll come up with a demo when I’m back at my desk.

The lambda design team knew this was a pain point, and considered some designs that could've alleviated the problem (http://mail.openjdk.java.net/pipermail/lambda-dev/2010-June/...). I think they decided it wouldn't work well, but it's at least possible to do things here.

That said, I think the decision to do lambdas the way they did tacitly killed checked exceptions.

You can't have an interface implementation throw exceptions not declared in the interface, hence all implementations of a given interface have to declare the same failure modes. And then everything is nicely coupled.

A great example of this is ByteArrayOutputStream's close(). It's documented to do literally nothing, and yet you still have to "handle" an IOException that will never, ever happen.

https://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayO...

The language doesn't force them to do that--they could override the interface and not declare the Exception. Then if you were working with an object declared to be ByteArrayOutputStream, you wouldn't need to handle the Exception. If working with the interface, you would, because that's the point of an interface.

Yes, this is a poor example. The declaration of that exception is either a mistake, or a way of reserving some wiggle room for future changes.

This is backwards of how I think about it, so I'm curious what you're thinking.

If an implementation of an interface can throw an exception that its interface doesn't declare, that breaks the abstraction. How can you safely use the interface? You either have to catch Exception (which is effectively the same as wrapping every exception in some InterfaceException class), or give up on handling any Exceptions thrown by the interface.

I don't think this is backward of your understanding ?

That's just how I phrased it 18 hours ago. The thing I find bad about it is not that exceptions have to be declared in the interface, that's good, it's that every implementations will have to declare them also, even in the case that a particular implementation has no failure mode.

This sounds like a misconception. Implementations don't have to declare the exceptions: https://news.ycombinator.com/item?id=23410443.

The interface can define a type parameter bounded to Throwable, and declare methods that throw that.

But in general, of course an implementation can't just randomly add checked exceptions, because that would violate the Liskov substitution principle.

I've run into this when working with lambdas in particular. Say for instance I want to track the performance of a particular bit of code in a standardized way. I can write a method that takes a supplier and handles the performance tracking while calling the supplier and returning the result. This way tracked functions don't have a load of eg. timer initialization etc. If the supplier throws a checked exception, it should be caught and handled by the code that actually cares about the call, not the performance wrapper.

Unfortunately, because of the way Java handles checked exceptions, I can't feed the supplier with a lambda or any other method reference that I'm aware of that throws a checked exception and let it be passed up to the original caller directly. So I need to catch my checked exceptions and wrap them in an unchecked exception to catch. Not pretty.

There's probably something I'm missing but the language certainly doesn't go out of its way to help with this sort of thing.

Java's checked exceptions pretty much died when they introduced lambdas. They don't work well together, at least not in the java type system.

Further, I'd say that IndexOutOfBoundsException can happen pretty much everywhere, so it does not make much sense to make it a checked exception anyway.

> Further, I'd say that IndexOutOfBoundsException can happen pretty much everywhere, so it does not make much sense to make it a checked exception anyway.

Indeed. I don't know of any programming languages in which out-of-bounds array access is an error whose handling is enforced by the type system (checked exception, result, etc). Ditto for numeric overflow and division by zero. I believe even Ada doesn't force error handling of these.

It’s a bit different, but languages like Idris have type systems that can prevent out of bounds array access. I’d be interested in seeing what the exception handling equivalent is.

If i had to guess: No relevant catch in the right place.

Perhaps there is no catch at all, so the systemUI process crashes. Since that process is critical, and responsible for things like the status bar, some supervisory process panics, and reboots the phone.

Alternatively perhaps a higher level catch clause does catch this error, but it is at too high a level to be able to recover cleanly by say displaying a black background instead of the wallpaper.

Hard to say without being more familar with the codebase.

It's been several years since I've used Java, but from what I remember most devs rarely catch unchecked exceptions (like out of bounds access) in their code.

You generally wouldn't want to catch any random exception you don't specifically know you should or need to catch, as that way lies the madness of working with corrupted data. Usually you'd have a toplevel handler as a final line of defense, but of course that doesn't help if the software automatically restore the state which makes it crash, then you just get a crash loop.

There is another way: Common Lisp and smalltalk both have resumable exceptions (although they’re more limited in smalltalk): in an interactive environment, if the exception isn’t handled, you drop into a debugger that lets you pick a recovery strategy. If something like this were standard on phones, you wouldn’t get stuck in a boot loop and Android could provide the option to enable dev mode or check the internet for a solution.

You can configure a JVM to pause whenever an exception is first-thrown and prompt to attach a debugger. Same with the CLR. I think Windows lets you do it with any program that uses SEH too.

Yeah but to most users, getting some cryptic console they can implement a workaround in ... isn’t much better.

It doesn’t need to be cryptic, a screen that says something like “enable dev mode” “attempt to fix from computer” (along with a desktop program for modifying settings) “check for updates” “reset to defaults” would be friendly enough

(In a better world, Android was written in Common Lisp.)

Newton almost had it.

Visual basic had an "ON ERROR RESUME NEXT" feature, which would simply ignore all exceptions and keep running code.

Obviously, when one thing goes wrong in a program, frequently that leads to other things going wrong, but IMO thats better than crashing the whole process.

Sure, you might get unexpected output, but for many usecases, unexpected output is better than no output at all.

Except maybe healthcare or finance or nuclear plant management software...

Turning off exceptions in this case doesn't prevent you from handling errors. You just have to check the error object to see.

No, this is not what the person who mentioned ON ERROR RESUME NEXT is arguing. They are arguing in some cases it's better if the program plods along and produces any output, even wrong output, rather than crashing.

I think the post above explains why this isn't the case in mission critical software. I'd argue the lesson from software engineering is that it's not advisable in most situations, not just for nuclear reactors. The error was raised by a reason, after all. I'd say it's the opposite: there are some limited cases where you absolutely know the exception is not relevant, and in those cases you can knowingly swallow the exception.

They stated that the feature "would simply ignore all exceptions". It allows you to ignore all exceptions, but you don't have to and probably wouldn't in most cases.

It was an example of blaming a tool for a hypothetical misuse that is not the only way you can use it.

I think the original poster implied one would indeed use it to ignore all exceptions. But also, it's exactly like an empty catch all exceptions block, which is considered a mistake today, for good reason.

I'll go an extra mile and say I'm familiar with this construct in Basic, have seen it used, and nobody ever checked any error codes when using it. It's a footgun that's used almost exclusively to shoot at feet.

Isn't it effectively the same thing as swallowing Throwable with an empty catch block in Java?

Yes, but you need to wrap every line of source code in your whole program (and all libraries) with an empty catch block to have the same result.

Just a single top level catch block would prevent other code after the error occurred from running, which might still be able to work fine without the results of the faulty code.

I see. I'm just not sure that situations where this way of exception handling is a viable strategy are common enough to warrant this syntactic sugar (which seems very easy to misuse). The Visual Basic docs also recommend using structured exception handling instead. Perhaps it is more useful in scripting contexts.

They are not viable strategies, which is why the analogous strategies in modern-day languages are heavily discouraged and wouldn't pass a code review.

> but IMO thats better than crashing the whole process

It's not. Crashing the process (if the error remains unhandled at the top level) is the right answer. When the program raises an exception, you want to either handle it or pass it up, not ignore it. It was raised for a reason. Once an error was raised, if the program ignores it and happily continues all bets are off. This is like reasoning out of false premises: everything goes, which is undesirable. Data is probably corrupted, maybe preconditions are unmet.

For this reason, most automated code checkers consider ignoring exceptions a serious antipattern.

It makes sense to catch any exception in certain specific places. For example in this case, wrap the entire batch of code for loading the file, parsing and processing it, and displaying on the screen in a try-catch in whatever code builds and displays the wallpaper. Then if anything goes wrong, you can log the error, and do some sane recovery steps such as removing that wallpaper and reverting to some default wallpaper that you know is good.

The solution is, at every layer of the system, to think about what it means to recover from a failure. In this case, the obvious answer to a failed wallpaper rendering is to avoid rendering the wallpaper, not to allow the failure to bring down the entire UI with it.

I didn't always have this habit, but picked it up when I switched from working on servers to working on safety-critical hardware. It was a lot easier a habit to pick up than I thought it would be, and I've found it makes you write a more robust, well-understood system.

That's good advice, to always be vigilant about how a system would recover from a possible error/failure, at every layer. I imagine this is a discipline that a software developer or engineer learns from experience, by running into edge cases, weird bugs, corrupted state, crashes, etc.

It's quite a responsibility on the human side of software. Handling unexpected errors is a pretty easy thing to miss, for programmers who are inexperienced or have become too comfortable (not paying attention to every detail).

I can't help but think that the language or compiler (or tests, I suppose) could enforce this better, so that it forces the programmer to explicitly handle recovery from failures - at certain layers/thresholds - so that the error doesn't bubble up to the top and bring the whole thing down.

As you pointed out, an error in the UI layer shouldn't have been allowed to trigger a system crash loop. But, that's kind of the nature of bugs like these, they creep in through unexpected logic paths. In which case, the development environment needs to better enforce the (explicit) behavior of all possible paths that an error could take. Well, I'm sure that's constantly improving, as a perennial challenge of reliable software, and there will always be a need for discipline and attentiveness by the humans involved.

> But, that's kind of the nature of bugs like these, they creep in through unexpected logic paths

Sure, agreed, this isn't a post-hoc "what an obvious bug, they should have just <done everything perfectly from the start>". I was just addressing the dichotomy laid out by the GP comment, which only mentioned top-level generic exception handling instead of case-specific, intelligent exception handling.

If the same thing happened in X trying to render an i3 background, for example, the way you'd recover is starting a new session - which you've proactively setup to not launch X automatically - and inspecting logs and configs to see what went wrong.

It's basically the same thing here, but without some physical keys bound to 'switch to new session'. And maybe you can do that over adb anyway, which is about as (not) normal user friendly.

Every time there's a new programming language which claims to be "safer" than existing ones, I look at what they do differently. In some cases, they've come up with a new way to restrict computation so that undefined/error cases aren't possible. In many cases, though, their solution is more along the lines of "don't do that", often with syntax that discourages (but doesn't forbid) it.

The trouble is that it's really easy to say "access the 10th position in a 5-element array". It's possible to make a programming language that doesn't allow you to say this, but it either makes it really awkward to do anything with arrays, or simply pushes the error somewhere else -- or both.

Well, if this process had been written in C, this may have been a security issue, so Java is still an improvement.

Being able to deny a victim access to their phone just by having them set a given wallpaper definitely IS already a security issue.

OK, fair enough. But you know what I meant. :-)

Java is "safer" in the sense that the error is defined. If you try accessing an array with an out of bounds index, you'll get this exception. A language is less safe than Java if the resulting behavior is undefined, or even worse, if it's random or depends on the content of some other memory.

Java doesn't claim it's magic. It's objectively safer by this measure. (More specifically, Java claims to be "memory safe" [1])

[1] https://en.wikipedia.org/wiki/Memory_safety

It's "safe" in that you can't read or write memory you're not supposed to be able to access. The program will crash, but at least you can't steal (or sabotage) data in memory.

Java only makes you declare checked exception not catch them. Its not _that_ difficult to end up with checked exceptions which get thrown all the way up to main and crash your program. IndexOutOfBounds isn't checked though so it can get thrown from your code or someone else's and crash the whole process. Java exception handling is a mess.

Reply on news.ycombinator.com