createImageBitmap doesn't work correctly with IPCBlob

RESOLVED FIXED in Firefox 59

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

Posted file test.html (obsolete) —
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.
Assignee: nobody → amarchesini
Priority: -- → P1
Sounds rather bad. Do we need the fix on beta too?
Posted patch mochitest (obsolete) — Splinter Review
Here a mochitest.
Attachment #8931436 - Attachment is obsolete: true
(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.
Attachment #8931438 - Attachment is obsolete: true
Attachment #8931503 - Flags: review?(aosmond)
Attachment #8931504 - Flags: review?(aosmond)
Posted patch part 3 - canvas/ImageBitmap (obsolete) — Splinter Review
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.
Attachment #8931508 - Flags: review?(aosmond)
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
Here an implementation of decodeImageBuffer. I'm waiting to see if green on try.
Attachment #8931608 - Flags: review?(aosmond)
Comment on attachment 8931608 [details] [diff] [review]
part 4 - decodeImageBuffer (optional)

Not green yet.
Attachment #8931608 - Flags: review?(aosmond)
Posted patch part 3 - canvas/ImageBitmap (obsolete) — Splinter Review
Attachment #8931508 - Attachment is obsolete: true
Attachment #8931508 - Flags: review?(aosmond)
Attachment #8931723 - Flags: review?(aosmond)
Attachment #8931608 - Attachment is obsolete: true
Attachment #8931724 - Flags: review?(aosmond)
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.
Attachment #8931504 - Flags: review?(aosmond) → review+
Attachment #8931503 - Flags: review?(aosmond) → review+
(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 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.
Attachment #8931724 - Flags: review?(aosmond) → review+
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.
> 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.
Attachment #8931723 - Attachment is obsolete: true
Attachment #8931723 - Flags: review?(aosmond)
Attachment #8931830 - Flags: review?(aosmond)
Duplicate of this bug: 1189632
I suspect this is the cause of bug 1241127 too?
Duplicate of this bug: 1241127
Attachment #8931830 - Flags: review?(aosmond) → review+
Pushed by amarchesini@mozilla.com:
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
Depends on: 1433037
You need to log in before you can comment on or make changes to this bug.