Closed Bug 1224647 Opened 4 years ago Closed 4 years ago

ImageBitmap code is doing multithreaded stuff with ErrorResult

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: bzbarsky, Assigned: kaku)

References

Details

Attachments

(2 files)

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...
Blocks: 1224664
Bug 1224647 - remove cross-threads usages of ErrorResult; r?bz
Attachment #8689410 - Flags: review?(bzbarsky)
(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.
Attachment #8689410 - Flags: review?(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.
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?
> 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.
(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?
Attachment #8689410 - Attachment description: MozReview Request: Bug 1224647 - remove cross-threads usages of ErrorResult; r?bz → MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r?bz
Attachment #8689410 - Flags: review?(bzbarsky)
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/
Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r?bz
Attachment #8690678 - Flags: review?(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.  :(
Attachment #8689410 - Flags: review?(bzbarsky) → review+
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.
Attachment #8690678 - Flags: review?(bzbarsky) → review+
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/
Attachment #8689410 - Attachment description: MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r?bz → MozReview Request: Bug 1224647 - part1 - remove ErrorResult in the creating ImageBitmap from ImageData code path; r=bz
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/
Attachment #8690678 - Attachment description: MozReview Request: Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r?bz → MozReview Request: Bug 1224647 - part2 - remove ErrorResult in the creating ImageBitmap from Blob code path; r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3751aad52208
https://hg.mozilla.org/mozilla-central/rev/f628ddbbbbff
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.