1420223 - createImageBitmap doesn't work correctly with IPCBlob

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P1)

Description

[Andrea Marchesini [:baku]]

Attachment: test.html

imgTools::DecodeImage doesn't work correctly with nsIAsyncInputStream because it uses ::Available to know the full size of the stream:
https://searchfox.org/mozilla-central/source/image/imgTools.cpp#50-52,73-75

IPCBlob exposes nsIAsyncInputStream and this makes createImageBitmap to fail.
I'm uploading a simple test to trigger this issue.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Sounds rather bad. Do we need the fix on beta too?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: mochitest

Here a mochitest.

Comment 3

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Sounds rather bad. Do we need the fix on beta too?

I think we should. Let's see how complex the fix will be.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - tests

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - imgITools::decodeImageAsync

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - canvas/ImageBitmap

In these 3 patches you can find:

1. DecodeImageAsync works on a separate thread. This means that the operation doesn't block the main-thread.

2. Here I had to rewrite CreateImageBitmap for workers because using a sync runnable we were working against the async aspect of Promise. Basically it was impossible to do other operations when CreateImageBitmap was running.

If you prefer (and I would like to), maybe we can move CreateImageBitmap logic in a separate file.

Comment 7

[Andrea Marchesini [:baku]]

aosmond, I'm thinking about a follow up to get rid of imgITool.decodeImage(inputStream) and introduce imITool.decodeImageFromBuffer(data, length). We should use it instead of decodeImage(inputStream) everywhere in our code if we want something sync and we are dealing with a buffer (this happens in many places how you can see in the following list). 

In general, treat inputStream as non-blocking and non-async is really bad.

Checking where decodeImage is used:

. toolkit/components/places/nsFaviconService.cpp - fine because we are using a NS_NewByteInputStream. We could use decodeImageFromBuffer.

. widget/windows/WinUtils.cpp - fine: also here we are using a NS_NewByteInputStream. Another good place where we could use decodeImageFromBuffer directly.

. dom/canvas/ImageBitmap.cpp - changed in this set of patches.

. toolkit/components/extensions/ext-clipboard.js - fine, we are reading from an ArrayBufferInputStream. We could directly decode using decodeImageFromBuffer.

. toolkit/mozapps/extensions/internal/LightweightThemeImageOptimizer.jsm - this is wrong! We are reading directly from the inputStream of a file. We could use decodeImageSync introduced by these patches.

. devtools/shared/gcli/commands/screenshot.js - wrong! We are reading a inputStream from the network (or in general from a NetUtil.newChannel). This should use decodeImageSync.

. browser/modules/WindowsPreviewPerTab.jsm - wrong. We read from a fetch inputStream. decodeImageSync.

Tests - we should care less:
. devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js
image/test/unit/test_imgtools.js

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 4 - decodeImageBuffer (optional)

Here an implementation of decodeImageBuffer. I'm waiting to see if green on try.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8931608 [details] [diff] [review]
part 4 - decodeImageBuffer (optional)

Not green yet.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 3 - canvas/ImageBitmap

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 4 - decodeImageBuffer

Comment 12

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

Comment on attachment 8931504 [details] [diff] [review]
part 2 - imgITools::decodeImageAsync

Review of attachment 8931504 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/imgITools.idl
@@ +35,5 @@
>  
>      /**
> +     * decodeImageAsync
> +     * See decodeImage. The main difference between this method and decodeImage
> +     * is that here the operation is done async on a separate thread. When the

nit: on a thread from the decode pool.

::: image/imgTools.cpp
@@ +40,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  ImageDecoderHelper(image::Image* aImage,

nit: already_AddRefed<image::Image>.

@@ +78,5 @@
> +      mCallback->OnImageReady(container, mStatus);
> +
> +      // Let's release data on the right thread.
> +      mCallback = nullptr;
> +      mImage = nullptr;

This runnable should be freed on the main thread if this is the last call. So an equivalent way of doing this (without a dispatch) but safer, is to do NS_ProxyRelease with the event target of the caller.

@@ +205,5 @@
> +
> +  // We don't want to block the main-thread. Please use DecodeImageAsync
> +  // instead.
> +  if (NS_WARN_IF(!nonBlocking)) {
> +    return NS_ERROR_FAILURE;

Is it possible (without breaking a ton of test cases) to add MOZ_ASSERT_UNREACHABLE in these async checks?

@@ +215,5 @@
>      nsCOMPtr<nsIInputStream> bufStream;
>      rv = NS_NewBufferedInputStream(getter_AddRefs(bufStream),
>                                     inStream.forget(), 1024);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    inStream = bufStream;

nit: move or forget.

@@ +239,5 @@
> +  if (length) {
> +    rv = image->OnImageDataAvailable(nullptr, nullptr, inStream, 0,
> +                                     uint32_t(length));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

Does the new check for length cause new behaviour / work around a bug? If not, remove it. If no bytes are written, OnImageDataAvailable should fail, and then we return an error. Otherwise we get to OnImageDataComplete which will try to sync decode the metadata, which in turn will fail and return an error code on the next line.

@@ +261,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  NS_ENSURE_ARG_POINTER(aInStr);
> +  NS_ENSURE_ARG_POINTER(aCallback);
> +  NS_ENSURE_ARG_POINTER(aEventTarget);

Ensure IsOnCurrentThread() is also true. It should also be true if the target is for the main thread as that is an assumption in ImageDecoderHelper::Run.

@@ +279,5 @@
> +    nsCOMPtr<nsIInputStream> bufStream;
> +    rv = NS_NewBufferedInputStream(getter_AddRefs(bufStream),
> +                                   stream.forget(), 1024);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    stream = bufStream;

nit: move or forget.

Comment 13

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

(In reply to Andrew Osmond [:aosmond] from comment #12)
> Comment on attachment 8931504 [details] [diff] [review]
> part 2 - imgITools::decodeImageAsync
> 
> Review of attachment 8931504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgITools.idl
> @@ +78,5 @@
> > +      mCallback->OnImageReady(container, mStatus);
> > +
> > +      // Let's release data on the right thread.
> > +      mCallback = nullptr;
> > +      mImage = nullptr;
> 
> This runnable should be freed on the main thread if this is the last call.
> So an equivalent way of doing this (without a dispatch) but safer, is to do
> NS_ProxyRelease with the event target of the caller.
> 

I meant call release in the destructor.

Comment 14

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

Comment on attachment 8931724 [details] [diff] [review]
part 4 - decodeImageBuffer

Review of attachment 8931724 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +100,5 @@
> +        }
> +
> +        callback(image);
> +      }
> +    };

I don't know enough about how it determines the types in .jsm files to properly review this part. In the image mochitests, we did something like this to get the correctly typed observer:

https://searchfox.org/mozilla-central/rev/4a590a5a15e35d88a3b23dd6ac3c471cf85b04a8/image/test/mochitest/test_bug399925.html#83

Does it automatically cast it to imgIContainerCallback? Similar comments for screenshot.js and LightweightThemeImageOptimizer.

Comment 15

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

Comment on attachment 8931723 [details] [diff] [review]
part 3 - canvas/ImageBitmap

Review of attachment 8931723 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageBitmap.cpp
@@ +1123,5 @@
> +    : CancelableRunnable("dom::CreateImageBitmapFromBlob")
> +    , mMutex("dom::CreateImageBitmapFromBlob::mMutex")
> +    , mPromise(aPromise)
> +    , mGlobalObject(aGlobal)
> +    , mBlob(&aBlob)

I realize this was the original code...but is this safe? The Blob in question does not seem to be allocated on the heap (it seems to come from a weird C++ union instead, similar to how Maybe works) and we are putting it into a RefPtr.

Comment 16

[Andrea Marchesini [:baku]]

> I realize this was the original code...but is this safe? The Blob in
> question does not seem to be allocated on the heap (it seems to come from a
> weird C++ union instead, similar to how Maybe works) and we are putting it
> into a RefPtr.

It's safe, but, just because Blob is non-thread-safe, I prefer to work with the inputStream since the CTOR.
I'm uploading a new version. Green on try.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 3 - canvas/ImageBitmap

Comment 19

[Anne (:annevk)]

I suspect this is the cause of bug 1241127 too?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb11906e602
createImageBitmap must work with nsIAsyncInputStream - part 1 - tests, r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb774ad4ae66
createImageBitmap must work with nsIAsyncInputStream - part 2 - imgITools::decodeImageAsync, r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/e257e633c87d
createImageBitmap must work with nsIAsyncInputStream - part 3 - ImageBitmap must use imgITools::decodeImageAsync, r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/e60f002f07d0
createImageBitmap must work with nsIAsyncInputStream - part 4 - ImageBitmap must use imgITools::decodeImageBuffer, r=aosmond

Comment 22

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5bb11906e602
https://hg.mozilla.org/mozilla-central/rev/bb774ad4ae66
https://hg.mozilla.org/mozilla-central/rev/e257e633c87d
https://hg.mozilla.org/mozilla-central/rev/e60f002f07d0

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/5bb11906e602
https://hg.mozilla.org/mozilla-central/rev/bb774ad4ae66
https://hg.mozilla.org/mozilla-central/rev/e257e633c87d
https://hg.mozilla.org/mozilla-central/rev/e60f002f07d0