Closed Bug 1452754 Opened 2 years ago Closed 2 years ago

Handle promise rejection in cases of error or otherwise through RAII in MapDataIntoBufferSource::DoMapDataIntoBufferSource

Categories

(Core :: Canvas: 2D, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(2 files)

The must-return-immediately infrastructure, added to allow the DOM error-result thing to have |aRv.Throw();| not forget to return afterward, allows RAII to occur between |aRv.Throw();| and the return.  This is good: the point of must-return-immediately is to make sure people don't just forget to return, and RAII is sort of an affirmative "I mean this stuff to happen".  Let's augment the existing test to verify this is the case, before I add code that specifically depends on it.
Comment on attachment 8966357 [details] [diff] [review]
Add tests to MustReturnFromCaller.cpp that verify that RAII destruction after a must-return-after expression is permitted

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

::: build/clang-plugin/tests/TestMustReturnFromCaller.cpp
@@ +9,5 @@
>  int MakeAnInt();
>  int MOZ_MAY_CALL_AFTER_MUST_RETURN SafeMakeInt();
>  bool Condition();
>  
> +template<typename Func>

Seems like it might be cleaner to just #include "mozilla/ScopeExit.h"
Attachment #8966357 - Flags: review?(nika) → review+
We can do that?  Fine by me, just didn't know that was kosher and would work.
Comment on attachment 8966358 [details] [diff] [review]
Handle promise rejection in cases of error or otherwise through RAII in MapDataIntoBufferSource::DoMapDataIntoBufferSource

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

::: dom/canvas/ImageBitmap.cpp
@@ +1653,5 @@
>    {
>      ErrorResult error;
>  
> +    auto rejectByDefault =
> +      MakeScopeExit([this, &error]() {

This lifetime is restricted to the local scope, so [&] would be fine.
Attachment #8966358 - Flags: review?(jgilbert) → review+
Mm, actually.  #include "mozilla/ScopeExit.h" will *also* define the two macro-attributes under test in this file -- so macro-redefinition errors seem like a concern.  Given that the prevailing clang-plugin test style is to define files that are as standalone as possible, it seems less-bad to have a 25-line implementation just in this file.  IMO.  Will land with that in place, despite comment 4.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51399503eb15
Add tests to TestMustReturnFromCaller.cpp that verify that RAII destruction after a must-return-after expression is permitted.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca65fd361e41
Handle promise rejection in cases of error or otherwise through RAII in MapDataIntoBufferSource::DoMapDataIntoBufferSource.  r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/51399503eb15
https://hg.mozilla.org/mozilla-central/rev/ca65fd361e41
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.