1224647 - ImageBitmap code is doing multithreaded stuff with ErrorResult

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

That's not supported, and it will completely explode on you depending on what gets thrown on the ErrorResult on the main thread.  You need to keep the ErrorResult uses thread-local.

Once this is fixed, I will try to add asserts to that effect to ErrorResult to keep this from being a problem in the future...

Comment 1

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #0)
> That's not supported, and it will completely explode on you depending on
> what gets thrown on the ErrorResult on the main thread.  You need to keep
> the ErrorResult uses thread-local.
> 
> Once this is fixed, I will try to add asserts to that effect to ErrorResult
> to keep this from being a problem in the future...

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz

Bug 1224647 - remove cross-threads usages of ErrorResult; r?bz

Comment 3

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #0)
> That's not supported, and it will completely explode on you depending on
> what gets thrown on the ErrorResult on the main thread.  You need to keep
> the ErrorResult uses thread-local.
> 
> Once this is fixed, I will try to add asserts to that effect to ErrorResult
> to keep this from being a problem in the future...

So, there is only one cross-threads usage in the CreateImageFromRawDataInMainThreadSyncTask. I removed it at the above patch.

Comment 4

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c675e7529500

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689410 [details]
MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz

https://reviewboard.mozilla.org/r/25631/#review23063

::: dom/canvas/ImageBitmap.cpp:157
(Diff revision 1)
> +  if (NS_WARN_IF(!image)) {

How can this be null?  CairoImage doesn't have a custom operator new, and this is not using fallible new.  AS far as I can tell, you can just leave this function as it was, except remove the ErrorResult argument.

::: dom/canvas/ImageBitmap.cpp:288
(Diff revision 1)
> +    MOZ_ASSERT(!(*aImage), "Pass an existing Image into CreateImageFromRawDataInMainThreadSyncTask.");

You mean "Don't pass"?

::: dom/canvas/ImageBitmap.cpp:694
(Diff revision 1)
> +    aRv = error; // Do not call Throw() because we might assign success codes.

No, this is not OK unless you know for a fact that the mainthread end of this never calls ThrowTypeError() or ThrowRangeError() or ThrowDOMException() or ThrowJSException() or NoteJSContextException() and never will.  Trying to do this with any of those error codes will assert.

Comment 6

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25631/#review23063

> How can this be null?  CairoImage doesn't have a custom operator new, and this is not using fallible new.  AS far as I can tell, you can just leave this function as it was, except remove the ErrorResult argument.

Ya, there is no way for the *image* to be null and remove the ErrorResult argument is good. I will do it.

> You mean "Don't pass"?

Yes and sorry.

> No, this is not OK unless you know for a fact that the mainthread end of this never calls ThrowTypeError() or ThrowRangeError() or ThrowDOMException() or ThrowJSException() or NoteJSContextException() and never will.  Trying to do this with any of those error codes will assert.

Understood. Luckily, in the main thread, only Throw() could be called on the ErrorResult object and the ErrorResult object is destructed after the error code was stolen. So, I think the code here is safe, maybe I can add a comment to describe the behavior here.

Or, I think it's better to remove the ErrorResult in the code path from CreateImageFromRawData() to CreateSurfaceFromRawData() and CreateImageFromSurface() so that we can keep this bad usage away.

What do u think?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Luckily, in the main thread, only Throw() could be called on the ErrorResult object

Right now.  My point is that this is fragile; someone could easily change the mainthread behavior without realizing how it affects the worker behavior.

> Or, I think it's better to remove the ErrorResult in the code path from
> CreateImageFromRawData() to CreateSurfaceFromRawData() and CreateImageFromSurface() so
> that we can keep this bad usage away.

If we can do that, that would be great.  If the code always does a Throw(), it can just be converted to using an nsresult return value for now.

The other option would be to come up with an explicit way to move an ErrorResult across threads, perhaps based on the existing IPC code for moving one across processes.  Not sure how much work that would be, and I'm definitely not asking you to sign up to do that.

Comment 8

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> > Or, I think it's better to remove the ErrorResult in the code path from
> > CreateImageFromRawData() to CreateSurfaceFromRawData() and CreateImageFromSurface() so
> > that we can keep this bad usage away.
> 
> If we can do that, that would be great.  If the code always does a Throw(),
> it can just be converted to using an nsresult return value for now.
I am going to take this approach in this bug. Since we only throw NS_ERROR_NOT_AVAILABLE, I will just check whether the return value is null or not without using a nsresult.

Should I open a new MozReview series? or keep appending on the current one?

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689410 [details]
MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25631/diff/1-2/

Comment 10

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r=bz

Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r?bz

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689410 [details]
MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz

https://reviewboard.mozilla.org/r/25631/#review23529

r=me.  Sorry for the lag responding to comment 8.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8690678 [details]
MozReview Request: Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r=bz

https://reviewboard.mozilla.org/r/25903/#review23531

r=me as long as those NS_ERROR_NOT_AVAILABLE can't happen in any cases per spec.

Comment 13

[Tzuhao Kuo [:kaku]]

Thanks for the review!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ea92459dd29

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689410 [details]
MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25631/diff/2-3/

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8690678 [details]
MozReview Request: Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25903/diff/1-2/

Comment 16

[Tzuhao Kuo [:kaku]]

Rebased and try again, which looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f60d4f70f6e9&selectedJob=14108581

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3751aad52208
https://hg.mozilla.org/integration/mozilla-inbound/rev/f628ddbbbbff

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3751aad52208
https://hg.mozilla.org/mozilla-central/rev/f628ddbbbbff