LibGfx/WebP: Redo error handling

Most places used to call `context.error()` to report an error,
which would set the context's state to `Error` and then return an
`Error::from_string_literal()`.

This is somewhat elegant, but it doesn't work: Some functions this
code calls returns ErrorOr<>s that aren't created by `context.error()`,
and for these we wouldn't enter the error state.

Instead, manually check error-ness at the leaf entry functions of the
class:

1. Add a set_error() helper for functions returning bool
2. In the two functions returning ErrorOr<>, awkwardly check the error
   manually.  If this becomes a very common pattern, maybe we can add
   a `TRY_WITH_HANDLER(expr, error_lambda)` which would invoke a
   lambda on error. We could use that here to set the error code.

No real behavior change (except we enter the error state more often
when something goes wrong).

2 files changed, 51 insertions, 25 deletions

diff --git a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp
index 854030cc83..923e7a2e0a 100644
--- a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp
+++ b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp
@@ -158,7 +158,7 @@ struct WebPLoadingContext {
     template<size_t N>
     [[nodiscard]] class Error error(char const (&string_literal)[N])
     {
-        state = WebPLoadingContext::State::Error;
+        // FIXME: Remove this function; make all callers call Error::from_string_literal() directly.
         return Error::from_string_literal(string_literal);
     }
 };
@@ -723,13 +723,22 @@ WebPImageDecoderPlugin::WebPImageDecoderPlugin(ReadonlyBytes data, OwnPtr<WebPLo
 
 WebPImageDecoderPlugin::~WebPImageDecoderPlugin() = default;
 
+bool WebPImageDecoderPlugin::set_error(ErrorOr<void>&& error_or)
+{
+    if (error_or.is_error()) {
+        m_context->state = WebPLoadingContext::State::Error;
+        return true;
+    }
+    return false;
+}
+
 IntSize WebPImageDecoderPlugin::size()
 {
     if (m_context->state == WebPLoadingContext::State::Error)
         return {};
 
     if (m_context->state < WebPLoadingContext::State::FirstChunkDecoded) {
-        if (decode_webp_first_chunk(*m_context).is_error())
+        if (set_error(decode_webp_first_chunk(*m_context)))
             return {};
     }
 
@@ -751,13 +760,15 @@ bool WebPImageDecoderPlugin::set_nonvolatile(bool& was_purged)
 
 bool WebPImageDecoderPlugin::initialize()
 {
-    return !decode_webp_header(*m_context).is_error();
+    return !set_error(decode_webp_header(*m_context));
 }
 
 bool WebPImageDecoderPlugin::sniff(ReadonlyBytes data)
 {
     WebPLoadingContext context;
     context.data = data;
+
+    // Intentionally no set_error() call: We're just sniffing `data` passed to the function, not reading m_context->data.
     return !decode_webp_header(context).is_error();
 }
 
@@ -773,7 +784,7 @@ bool WebPImageDecoderPlugin::is_animated()
         return false;
 
     if (m_context->state < WebPLoadingContext::State::FirstChunkDecoded) {
-        if (decode_webp_first_chunk(*m_context).is_error())
+        if (set_error(decode_webp_first_chunk(*m_context)))
             return false;
     }
 
@@ -786,7 +797,7 @@ size_t WebPImageDecoderPlugin::loop_count()
         return 0;
 
     if (m_context->state < WebPLoadingContext::State::AnimationFrameChunksDecoded) {
-        if (decode_webp_animation_frame_chunks(*m_context).is_error())
+        if (set_error(decode_webp_animation_frame_chunks(*m_context)))
             return 0;
     }
 
@@ -799,7 +810,7 @@ size_t WebPImageDecoderPlugin::frame_count()
         return 1;
 
     if (m_context->state < WebPLoadingContext::State::ChunksDecoded) {
-        if (decode_webp_chunks(*m_context).is_error())
+        if (set_error(decode_webp_chunks(*m_context)))
             return 1;
     }
 
@@ -819,35 +830,48 @@ ErrorOr<ImageFrameDescriptor> WebPImageDecoderPlugin::frame(size_t index)
     if (m_context->state == WebPLoadingContext::State::Error)
         return Error::from_string_literal("WebPImageDecoderPlugin: Decoding failed");
 
-    if (m_context->state < WebPLoadingContext::State::ChunksDecoded)
-        TRY(decode_webp_chunks(*m_context));
+    // In a a lambda so that only one check to set State::Error is needed, instead of one per TRY.
+    auto decode_frame = [this](size_t index) -> ErrorOr<ImageFrameDescriptor> {
+        if (m_context->state < WebPLoadingContext::State::ChunksDecoded)
+            TRY(decode_webp_chunks(*m_context));
 
-    if (is_animated()) {
-        if (m_context->state < WebPLoadingContext::State::AnimationFrameChunksDecoded)
-            TRY(decode_webp_animation_frame_chunks(*m_context));
-        return decode_webp_animation_frame(*m_context, index);
-    }
+        if (is_animated()) {
+            if (m_context->state < WebPLoadingContext::State::AnimationFrameChunksDecoded)
+                TRY(decode_webp_animation_frame_chunks(*m_context));
+            return decode_webp_animation_frame(*m_context, index);
+        }
+
+        if (m_context->state < WebPLoadingContext::State::BitmapDecoded) {
+            auto bitmap = TRY(decode_webp_image_data(*m_context, m_context->image_data.value()));
 
-    if (m_context->state < WebPLoadingContext::State::BitmapDecoded) {
-        auto bitmap = TRY(decode_webp_image_data(*m_context, m_context->image_data.value()));
+            // Check that size in VP8X chunk matches dimensions in VP8 or VP8L chunk if both are present.
+            if (m_context->first_chunk->type == FourCC("VP8X")) {
+                if (static_cast<u32>(bitmap->width()) != m_context->vp8x_header.width || static_cast<u32>(bitmap->height()) != m_context->vp8x_header.height)
+                    return m_context->error("WebPImageDecoderPlugin: VP8X and VP8/VP8L chunks store different dimensions");
+            }
 
-        // Check that size in VP8X chunk matches dimensions in VP8 or VP8L chunk if both are present.
-        if (m_context->first_chunk->type == FourCC("VP8X")) {
-            if (static_cast<u32>(bitmap->width()) != m_context->vp8x_header.width || static_cast<u32>(bitmap->height()) != m_context->vp8x_header.height)
-                return m_context->error("WebPImageDecoderPlugin: VP8X and VP8/VP8L chunks store different dimensions");
+            m_context->bitmap = move(bitmap);
+            m_context->state = WebPLoadingContext::State::BitmapDecoded;
         }
 
-        m_context->bitmap = move(bitmap);
-        m_context->state = WebPLoadingContext::State::BitmapDecoded;
-    }
+        VERIFY(m_context->bitmap);
+        return ImageFrameDescriptor { m_context->bitmap, 0 };
+    };
 
-    VERIFY(m_context->bitmap);
-    return ImageFrameDescriptor { m_context->bitmap, 0 };
+    auto result = decode_frame(index);
+    if (result.is_error()) {
+        m_context->state = WebPLoadingContext::State::Error;
+        return result.release_error();
+    }
+    return result.release_value();
 }
 
 ErrorOr<Optional<ReadonlyBytes>> WebPImageDecoderPlugin::icc_data()
 {
-    TRY(decode_webp_chunks(*m_context));
+    if (auto result = decode_webp_chunks(*m_context); result.is_error()) {
+        m_context->state = WebPLoadingContext::State::Error;
+        return result.release_error();
+    }
 
     // FIXME: "If this chunk is not present, sRGB SHOULD be assumed."
 
diff --git a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.h b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.h
index 50fd90bb56..7365ef7dc6 100644
--- a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.h
+++ b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.h
@@ -31,6 +31,8 @@ public:
     virtual ErrorOr<Optional<ReadonlyBytes>> icc_data() override;
 
 private:
+    bool set_error(ErrorOr<void>&&);
+
     WebPImageDecoderPlugin(ReadonlyBytes, OwnPtr<WebPLoadingContext>);
 
     OwnPtr<WebPLoadingContext> m_context;