1331434 - add a must-return-from-calling block static analysis check

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment 8827099 [details] [diff] fixed a thinko that was resulting in a crash; we ought to have been able to catch this programming mistake with static analysis.

I think we want something like MOZ_MUST_RETURN_FROM_BLOCK, where the static analysis would check that if you called a function/method annotated thusly, it is an error to not return from the block containing the relevant call.

Ehsan, does that sound correct?

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP Patch

I threw together a basic patch to get an idea as to the feasibility of this check.

This doesn't handle every case which we would care about, for example, it would complain in this situation:

void foo(ErrorResult& aRv) {
  aRv.Throw(NS_ERROR_UNIMPLEMENTED);
}

and in this situation:

void bar(ErrorResult& aRv) {
  bool succeeded = TryToDoSomething();
  if (!succeeded) {
    aRv.Throw(NS_ERROR_FAILURE);
  }
}

and explicit `return;`s would need to be added after each of the calls to Throw. I imagine we would want to not complain about that situation.

MozReview-Commit-ID: 7NqXap8FdSn

Comment 2

[Nathan Froyd [:froydnj]]

Thanks for throwing those patches together!

To handle those situations, we'd need something more like a flow analysis: the only code path reachable after MOZ_MUST_RETURN_FROM_CALLER is a return.  I think requiring an explicit `return` in the second case is not a particularly big deal and would arguably make us a little more consistent.

For the first case, it looks like requiring an explicit `return` would affect ~10 calls of .Throw() out of ~1700+, which doesn't seem terrible.  For a lot of those cases, we could refine the analysis to simply check if the must-return function is the last call in the only block of the function and the calling function returns void--or something like that?  I think that would catch the vast majority of cases, and we'd only have to rewrite one or two functions.

WDYT?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Partial changes to existing code

I wrote a simple change to handle the first case but not the second one, and then started seeing how many changes would need to be made for the pass to run on M-C.

I have already found a few bugs with this analysis where we perform checks for null, and call aRv.Throw(), then continue on and dereference the null pointer, but I've also found just a ton of examples where we are adding unnecessary return; statements.

I also worry that this will cause otherwise good patches to fail when landed, increasing backouts, as this can cause some intuitively correct code to fail to build in S-A builds.

This is just some of the failing cases fixed, I haven't fixed even close to all of them. I'd love feedback as to whether or not you think it would be worthwhile to continue persuing this.


MozReview-Commit-ID: 4Rf1o0veHnC

Comment 4

[(no longer active)]

I think the idea here is good, but the patch needs to improve a lot.  Besides the cases covered in comment 2, see the ones below.  A correct analysis should leave them all untouched.

 Animation::GetReady(ErrorResult& aRv)
 {
   nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
   if (!mReady && global) {
     mReady = Promise::Create(global, aRv); // Lazily create on demand
   }
   if (!mReady) {
     aRv.Throw(NS_ERROR_FAILURE);
+    return nullptr;
   } else if (PlayState() != AnimationPlayState::Pending) {
     mReady->MaybeResolve(this);
   }
   return mReady;
 }

 void
 Animation::SetCurrentTimeAsDouble(const Nullable<double>& aCurrentTime,
                                         ErrorResult& aRv)
 {
   if (aCurrentTime.IsNull()) {
     if (!GetCurrentTime().IsNull()) {
       aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
+      return;
     }
     return;
   }
 
   return SetCurrentTime(TimeDuration::FromMilliseconds(aCurrentTime.Value()));
 }

     return keyframes;
   }
 
   // FIXME: Bug 1311257: Support missing keyframes for Servo backend.
   if ((!AnimationUtils::IsCoreAPIEnabled() ||
        aDocument->IsStyledByServo()) &&
       RequiresAdditiveAnimation(keyframes, aDocument)) {
     aRv.Throw(NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR);
-    keyframes.Clear();
+    return nsTArray<Keyframe>();
   }
 
   return keyframes;
 }

   TimingParams result;
   if (aOptions.IsUnrestrictedDouble()) {
     double durationInMs = aOptions.GetAsUnrestrictedDouble();
     if (durationInMs >= 0) {
       result.mDuration.emplace(
         StickyTimeDuration::FromMilliseconds(durationInMs));
     } else {
       aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
+      return result;
     }
   } else {
     const dom::AnimationEffectTimingProperties& timing =
       GetTimingProperties(aOptions);
 
     Maybe<StickyTimeDuration> duration =
       TimingParams::ParseDuration(timing.mDuration, aRv);
     if (aRv.Failed()) {

I could go on and on, but it's clear that currently the patch is mostly producing false positives.  Without proper data flow analysis (since you don't want to add a return statement which doesn't change the meaning of the program) it's unclear to me whether we can do something better though.  :/

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add an analysis to require a return after calls to annotated functions

I got the analysis to build on linux so I'm attaching that version.

Ehsan, how do you feel about this analysis? I am worried that it is too noisy, but if you look at the second patch you can see that I've caught quite a few small bugs with it.

MozReview-Commit-ID: 7NqXap8FdSn

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Mark ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

MozReview-Commit-ID: CPEAO9OhLG

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #4)
> I think the idea here is good, but the patch needs to improve a lot. 
> Besides the cases covered in comment 2, see the ones below.  A correct
> analysis should leave them all untouched.
> 
...
> 
> I could go on and on, but it's clear that currently the patch is mostly
> producing false positives.  Without proper data flow analysis (since you
> don't want to add a return statement which doesn't change the meaning of the
> program) it's unclear to me whether we can do something better though.  :/

Oops, I missed your message.

I agree that most of the changes which I am making shouldn't be necessary, but I am not sure how else it would work.

Basically my opinion is that this analysis is too noisy and would take some substantial effort to clean up the noise of, _but_ it did uncover bugs. The question I suppose is about balancing those two factors.

Comment 8

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #7)
> Basically my opinion is that this analysis is too noisy and would take some
> substantial effort to clean up the noise of, _but_ it did uncover bugs. The
> question I suppose is about balancing those two factors.

We can't land this in its current form, because it will make it impossible to use the ErrorResult type unusable, since pretty much any complex code that uses Throw() will be rejected by the static analysis.

What are the real bugs that this has found?  Do you have a sense of the number of them vs the total number of issues reported?  I skimmed the resulting m-c changes a bit and it seemed like everything I'm looking at is a false positive.  :(

I'd also like to know what Boris thinks about this since he designed the ErrorResult API.

Comment 9

[(no longer active)]

Comment on attachment 8827573 [details] [diff] [review]
Mark ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

Clearing since this patch isn't ready for review.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What are the real bugs that this has found?

Well, that very first bit in accessible/aom/AccessibleNode.cpp is kinda buggy.  Not least because if !mIntl it will null-deref!

The second call-without-return in that same function is OK in practice, for a slightly subtle reason: yes, aValue does get changed, but it's ignored by the caller if aRv is a failure, so it doesn't matter that it got changed.  I do think that both return statements should be there in that function, to make it clear what's going on.

I think FileReader::ReadFileContent is buggy too; it will helpfully ignore that mFileData allocation failed and press on, clobbering the ErrorResult with possibly-ok.

There might be others; I stopped skimming about halfway through.

OK, on to my actual thoughts...

The CallbackObject::CallSetup::~CallSetup change shows one problem that I was wondering about: there are times when a Throw() call on an ErrorResult does _not_ mean "we must bail out right now".  This can even be the case for an outparam ErrorResult, if there is some sort of non-RAII cleanup that still needs to happen (which is the situation in CallbackObject there, kinda).  I'm not quite sure how best to handle that.  But we need _something_, because while the changes to CallbackObject are sort of ok, if not great, the changes to ScriptExecutorRunnable::ShutdownScriptLoader are probably not.  Yes, we always want to stop the syncloop.  And I'm almost entirely sure we went to do it _after_ we have put mScriptLoader.mRv in the correct state, because the StopSyncLoop call will trigger code that expects  that object in that state, and I _think_ it triggers it synchronously.

In general, some sort of analysis like this _would_ be a good idea; we've had too many cases of people throwing on an ErrorResult and then pressing on (again, see the accessibility code with its potential null-deref).  The noise due to trailing blocks in void functions is really unfortunate; on the other hand it prevents someone adding something to the end of such a function and suddenly ending up with the "pressing on" situation...

If we _can_ do the flow-type analysis Ehsan suggests, that would clearly be better.  We'd need ways to opt out or indicate that we know what we're doing in cases like CallbackObject and ScriptExecutorRunnable.

Comment 11

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> If we _can_ do the flow-type analysis Ehsan suggests, that would clearly be
> better.

Clang includes some path sensitive analysis support which we should be able to use here.  See <https://clang-analyzer.llvm.org/checker_dev_manual.html>.  What we want to do is to generate all of the paths through the function being analyzed, and look for paths which have statements after the one with the .Throw() call, and if we find _a_ path matching that condition, issue an error.

Looking at the code, I think a good place to start is AnalysisConsumer.  This class is the AST consumer driving the clang-analyzer checks.  Specifically, see AnalysisConsumer::HandleCode() which is the starting point for a function definition which in turn calls RunPathSensitiveChecks.  I would imagine we'd want to reuse code such as CoreEngine/ExprEngine, but since this will be the first analysis doing any path sensitive operations you should explore things a bit. :-)

>We'd need ways to opt out or indicate that we know what we're doing
> in cases like CallbackObject and ScriptExecutorRunnable.

I suppose one easy way to deal with this is to have another variant of Throw() like ThrowWithCustomCleanup() which is exempted from the checks here and use it in places where we expect to do custom non-RAII cleanup.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to require a return after calls to annotated functions

Hey BZ, I took a shot at implementing an analysis for this which tries to accept most OK situations, while rejecting the problematic ones. To get an idea of that this analysis would reject please look at the Test file.

I'd appreciate input from you wrt whether or not this analysis works the way it should. You can also look at the second patch, which involves me running the analysis on the tree and making the changes required by it.

MozReview-Commit-ID: 7NqXap8FdSn

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

MozReview-Commit-ID: GfnX0cylirE

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8829977 [details] [diff] [review]
Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

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

::: dom/media/webaudio/ScriptProcessorNode.h
@@ +98,5 @@
>    void SetChannelCount(uint32_t aChannelCount, ErrorResult& aRv) override
>    {
>      if (aChannelCount != ChannelCount()) {
>        aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +      return;

This was an unnecessary change. Don't know why I didn't catch it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8829977 [details] [diff] [review]
Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

The change in StructuredCloneHolder::Read is probably wrong: we want to clear things even if we threw, if mSupportsTransferring.  At least I think we do.

I think I'd prefer ThrowWithCustomCleanup instead of ThrowWithCleanup.

We can have a followup to add this annotation to the other ErrorResult Throw* methods, right?

>+++ b/dom/canvas/ImageBitmap.cpp

The changes here are fine as far as they go, but I don't think this code should have been using ErrorResult to start with.  It can just pass nsresult to mPromise->MaybeReject() and get the same outcome with a lot less mental overhead (and a bit less execution time).  Maybe file a followup?

>@@ -245,18 +245,20 @@ WebGLContext::GetVertexAttrib(JSContext* cx, GLuint index, GLenum pname,

Could switch to JS::ObjectValue(*obj) here, right?

>@@ -1188,17 +1188,17 @@ FlyWebService::PairWithService(const nsAString& aServiceId,

Uh...  This code is complete nonsense.  It's passing an already-failing ErrorResult to something that should be an _outparam_.  Please file a followup bug to get some sanity here?  And in general, the uses of ENSURE_SUCCESS_VOID in flyweb code are almost all busted because they don't clean up the ErrorResult.  Another followup?

I filed bug 1333556 to assert on the "throw on the outparam in the caller" thing.

>@@ -584,16 +584,17 @@ WorkerMainThreadRunnable::Dispatch(Status aFailStatus, ErrorResult& aRv)

Pretty sure the change here is wrong too.  We want to accumulate telemetry even if the syncloop got interrupted.

Overall this looks pretty good.  Caught a few more bugs!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8829976 [details] [diff] [review]
Part 1: Add an analysis to require a return after calls to annotated functions

So a question about the TestMustReturnFromCaller file.  This test:

>+int a4() {
>+  Throw();
>+  return Condition() ? MakeAnInt() : MakeAnInt();
>+}

is that expected to be accepted by the analysis?  Why?  Conceptually it seems a bit bogus to allow that, but if it's just a matter of us not having a good way to prevent it, then I can live with that.

Looking at the Firefox patch, the fact that we had to change Animation::GetReady is a bit annoying, in that the code there is conceptually fine.  Similar for Navigator::GetMozPower...  I'm not sure it's easy to avoid that, but if possible would be nice.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
> Comment on attachment 8829976 [details] [diff] [review]
> Part 1: Add an analysis to require a return after calls to annotated
> functions
> 
> So a question about the TestMustReturnFromCaller file.  This test:
> 
> >+int a4() {
> >+  Throw();
> >+  return Condition() ? MakeAnInt() : MakeAnInt();
> >+}
> 
> is that expected to be accepted by the analysis?  Why?  Conceptually it
> seems a bit bogus to allow that, but if it's just a matter of us not having
> a good way to prevent it, then I can live with that.

What I'm allowing is the following:
Any expression which has a `return` as a parent statement, so long as it is in the same { ... } block as the Throw() call.
Any expression which has a `return` as a parent statement in a different { ... } block, so long as it only contains calls to Move constructors, or constructors which are being passed the `nullptr` literal as their only argument. (These are usually implicit).

This allows the ternary because it is wholly contained by the return statement. I could place some arbitrary limit on the complexity of the expression in the return statement, but I'm not sure how well that would work.

> Looking at the Firefox patch, the fact that we had to change
> Animation::GetReady is a bit annoying, in that the code there is
> conceptually fine.  Similar for Navigator::GetMozPower...  I'm not sure it's
> easy to avoid that, but if possible would be nice.

This could be fixed I think by allowing implicit Copy constructors as well as Move constructors in not-same-block `return` statements.

Do you think that would be a good idea?

Comment 18

[(no longer active)]

What are the specific constructors (copy and move) that you encountered in the code base that led to these rules?

Comment 19

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> Comment on attachment 8829977 [details] [diff] [review]
> Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER
> 
> The change in StructuredCloneHolder::Read is probably wrong: we want to
> clear things even if we threw, if mSupportsTransferring.  At least I think
> we do.
> 
> I think I'd prefer ThrowWithCustomCleanup instead of ThrowWithCleanup.
> 
> We can have a followup to add this annotation to the other ErrorResult
> Throw* methods, right?

Yup! I just decided to do it to the core method in this patch.

> >+++ b/dom/canvas/ImageBitmap.cpp
> 
> The changes here are fine as far as they go, but I don't think this code
> should have been using ErrorResult to start with.  It can just pass nsresult
> to mPromise->MaybeReject() and get the same outcome with a lot less mental
> overhead (and a bit less execution time).  Maybe file a followup?

Sure, sgtm.

> >@@ -245,18 +245,20 @@ WebGLContext::GetVertexAttrib(JSContext* cx, GLuint index, GLenum pname,
> 
> Could switch to JS::ObjectValue(*obj) here, right?

Yes, we should be able to do that. I'll make the change.

> >@@ -584,16 +584,17 @@ WorkerMainThreadRunnable::Dispatch(Status aFailStatus, ErrorResult& aRv)
> 
> Pretty sure the change here is wrong too.  We want to accumulate telemetry
> even if the syncloop got interrupted.

OK, can change.

> Overall this looks pretty good.  Caught a few more bugs!

Comment 20

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #18)
> What are the specific constructors (copy and move) that you encountered in
> the code base that led to these rules?

    already_AddRefed<Foo>
    DoStuff(ErrorResult& aRv) {
      if (ErrorCondition()) {
        aRv.Throw(NS_ERROR_FAILURE);
      }
      return nullptr;
    }

generates CFG nodes for

    already_AddRefed<Foo>(nullptr_t)

to construct the already_AddRefed<T> from the nullptr, and:

    already_AddRefed<Foo>(already_AddRefed<Foo>&&)

to move the temporary into the caller.

I wanted situations like that to be OK, so I added in these rules.

Comment 21

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #20)
> (In reply to :Ehsan Akhgari from comment #18)
> > What are the specific constructors (copy and move) that you encountered in
> > the code base that led to these rules?
> 
>     already_AddRefed<Foo>
>     DoStuff(ErrorResult& aRv) {
>       if (ErrorCondition()) {
>         aRv.Throw(NS_ERROR_FAILURE);
>       }
>       return nullptr;
>     }
> 
> generates CFG nodes for
> 
>     already_AddRefed<Foo>(nullptr_t)
> 
> to construct the already_AddRefed<T> from the nullptr, and:
> 
>     already_AddRefed<Foo>(already_AddRefed<Foo>&&)
> 
> to move the temporary into the caller.

Makes sense.

> I wanted situations like that to be OK, so I added in these rules.

I think we want something slightly different here.  I think all ctors that can be invoked implicitly are fine to ignore, and anything else is not.  For example, the first ctor above is marked as MOZ_IMPLICIT.  The second ctors is a move ctor, therefore it can be ignored.  I'm wondering if the set of stuff caught by the analysis will change based on this.  Can you please give that a try?

Comment 22

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #17)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
> > Comment on attachment 8829976 [details] [diff] [review]
> > Part 1: Add an analysis to require a return after calls to annotated
> > functions
> > 
> > So a question about the TestMustReturnFromCaller file.  This test:
> > 
> > >+int a4() {
> > >+  Throw();
> > >+  return Condition() ? MakeAnInt() : MakeAnInt();
> > >+}
> > 
> > is that expected to be accepted by the analysis?  Why?  Conceptually it
> > seems a bit bogus to allow that, but if it's just a matter of us not having
> > a good way to prevent it, then I can live with that.
> 
> What I'm allowing is the following:
> Any expression which has a `return` as a parent statement, so long as it is
> in the same { ... } block as the Throw() call.
> Any expression which has a `return` as a parent statement in a different {
> ... } block, so long as it only contains calls to Move constructors, or
> constructors which are being passed the `nullptr` literal as their only
> argument. (These are usually implicit).
> 
> This allows the ternary because it is wholly contained by the return
> statement. I could place some arbitrary limit on the complexity of the
> expression in the return statement, but I'm not sure how well that would
> work.

Another case to think about here Boris is:

  void foo() {
    Throw();
    return SomethingWithSuperBadSideEffects();
  }

We currently accept this, and we sort of used the same argument to allow the ternary expression, although my immediate reaction to that was similar to yours.

I had also suggested to Michael to see if he can "inline" the methods in the return expression to detect cases where we run into something side-effect-full but that should be a good bit more complicated to implement.  I personally only have an extremely hand wavy idea how to do the said inlining at the CFG level.  And clang's code where they do similar inlining tricks on the C++ side wasn't super easy to follow...

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #22)
> Another case to think about here Boris is:
> 
>   void foo() {
>     Throw();
>     return SomethingWithSuperBadSideEffects();
>   }
> 
> We currently accept this, and we sort of used the same argument to allow the
> ternary expression, although my immediate reaction to that was similar to
> yours.

ISTR clang whining about this pattern of returning with a void function under our -Wall -Werror flags.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> This allows the ternary because it is wholly contained by the return statement.

Hmm.  It's annoying because it can include arbitrary side-effects in there, but I'm not sure whether there's a good way to avoid that...

> This could be fixed I think by allowing implicit Copy constructors as well as Move constructors

That seems reasonable.

> Another case to think about here Boris is:

Yes, that case seems bad to me too, and I would love to forbid it we can figure out a way to do it....

Comment 25

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #23)
> (In reply to :Ehsan Akhgari from comment #22)
> > Another case to think about here Boris is:
> > 
> >   void foo() {
> >     Throw();
> >     return SomethingWithSuperBadSideEffects();
> >   }
> > 
> > We currently accept this, and we sort of used the same argument to allow the
> > ternary expression, although my immediate reaction to that was similar to
> > yours.
> 
> ISTR clang whining about this pattern of returning with a void function
> under our -Wall -Werror flags.

Oh sorry the void part was a typo.  :(

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24)
> > This allows the ternary because it is wholly contained by the return statement.
> 
> Hmm.  It's annoying because it can include arbitrary side-effects in there,
> but I'm not sure whether there's a good way to avoid that...

Maybe we can special case the ternary operator?

> > Another case to think about here Boris is:
> 
> Yes, that case seems bad to me too, and I would love to forbid it we can
> figure out a way to do it....

The difficulty starts with defining what a side effect means exactly.  I'm not sure if I have a good definition...

Comment 26

[Boris Zbarsky [:bzbarsky]]

So here's a question.  If we only allow member reads (can we detect those?), stack reads (can we detect those?) and the implicit copy/move ctors, would that get us closer to what we want?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I guess no, for cases when we need to return a Nullable<int32>() or whatnot.  :(

Comment 28

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #27)
> I guess no, for cases when we need to return a Nullable<int32>() or whatnot.
> :(

I had the idea at one point that we could whitelist a small subset of expressions (so long as they are in a return statement):

Constructor calls (I was thinking all constructor invocations, we could limit to default, copy, and move constructors if we want)
DeclRefExpr (references to local, static, or member variables)
(maybe) calls to a subset of allowed methods (perhaps "RefPtr::forget()" could be whitelisted? We'll see what else we run into).

How does that sound?

Comment 29

[Boris Zbarsky [:bzbarsky]]

That sounds pretty awesome if it works!

Comment 30

[(no longer active)]

It sounds good to me on paper at least.  I'm a bit worried about inventing too much of a magic rule here because programmers need to understand what is and is not acceptable, but I'd like to see how your suggestion changes the list of issues found before making a judgement.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to require a return after calls to annotated functions

This is an updated version of the analysis which does the things which i suggested in the comment earlier. It seems to have reduced the number of necessary changes. DO you like this better?

MozReview-Commit-ID: 7NqXap8FdSn

Comment 32

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

The changes

MozReview-Commit-ID: GfnX0cylirE

Comment 33

[(no longer active)]

It seems like the new patch made these changes go away: Navigator::GetMozPower, nsImageLoadingContent.cpp, HTMLCanvasElement::TransferControlToOffscreen, HTMLInputElement.cpp, HTMLTextAreaElement.cpp, ScriptProcessorNode.h, DOMSVGLengthList.cpp, DOMSVGNumberList.cpp, DOMSVGPathSegList.cpp, DOMSVGPointList.cpp and DOMSVGTransformList.cpp.

This looks fantastic to me, great work!

Boris, are you happy with these changes?

Comment 34

[Boris Zbarsky [:bzbarsky]]

I didn't red over the new part 2 patch very carefully, but I agree that this looks great based on some spot-checking.  Let's ship it.

Comment 35

[(no longer active)]

Comment on attachment 8835017 [details] [diff] [review]
Part 1: Add an analysis to require a return after calls to annotated functions

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

Looks great, thanks again!

::: build/clang-plugin/CustomMatchers.h
@@ +231,5 @@
>    const CXXMethodDecl *Decl = Node.getCanonicalDecl();
>    return Decl && !Decl->isVirtual();
>  }
> +
> +AST_MATCHER(FunctionDecl, isMozMustReturnFromCaller) {

Why the "Moz"?  How about mustReturnFromCaller?

::: build/clang-plugin/MustReturnFromCallerChecker.cpp
@@ +91,5 @@
> +        continue;
> +      }
> +    }
> +
> +    AfterTrivials->dump();

This looks like debugging output left-over?

::: build/clang-plugin/Utils.h
@@ +418,5 @@
> +// contained in more than one `CFGBlock`; in this case, they are mapped to the
> +// innermost block (i.e. the one that is furthest from the root of the tree).
> +// An optional outparameter provides the index into the block where the `Stmt`
> +// was found.
> +class StmtToBlockMap {

Do you mind moving both this and RecurseGuard each to their own header please?

::: mfbt/Attributes.h
@@ +514,5 @@
> + *   overridden counterparts. This marker indicates that the marked method must
> + *   be called by the method that it overrides.
> + * MOZ_MUST_RETURN_FROM_CALLER: Applies to function or method declarations.
> + *   Callers of the annotated function/method must return from that function
> + *   within the calling block using an explicit `return` statement.

Did you forget MOZ_MAY_CALL_AFTER_MUST_RETURN or do you want to add that later?  I think we may as well add it here.

Comment 36

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #35)
> ::: build/clang-plugin/Utils.h
> @@ +418,5 @@
> > +// contained in more than one `CFGBlock`; in this case, they are mapped to the
> > +// innermost block (i.e. the one that is furthest from the root of the tree).
> > +// An optional outparameter provides the index into the block where the `Stmt`
> > +// was found.
> > +class StmtToBlockMap {
> 
> Do you mind moving both this and RecurseGuard each to their own header
> please?

Should I only include them in the place where I'm using them, or should they be included in Utils.h? I remember you had worries about this code both being built in unified and non-unified mode.

> ::: mfbt/Attributes.h
> @@ +514,5 @@
> > + *   overridden counterparts. This marker indicates that the marked method must
> > + *   be called by the method that it overrides.
> > + * MOZ_MUST_RETURN_FROM_CALLER: Applies to function or method declarations.
> > + *   Callers of the annotated function/method must return from that function
> > + *   within the calling block using an explicit `return` statement.
> 
> Did you forget MOZ_MAY_CALL_AFTER_MUST_RETURN or do you want to add that
> later?  I think we may as well add it here.

I add the documentation for it in the next patch because I'm bad at rebasing. I'll fix that before I land it.

Comment 37

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #36)
> (In reply to :Ehsan Akhgari from comment #35)
> > ::: build/clang-plugin/Utils.h
> > @@ +418,5 @@
> > > +// contained in more than one `CFGBlock`; in this case, they are mapped to the
> > > +// innermost block (i.e. the one that is furthest from the root of the tree).
> > > +// An optional outparameter provides the index into the block where the `Stmt`
> > > +// was found.
> > > +class StmtToBlockMap {
> > 
> > Do you mind moving both this and RecurseGuard each to their own header
> > please?
> 
> Should I only include them in the place where I'm using them, or should they
> be included in Utils.h? I remember you had worries about this code both
> being built in unified and non-unified mode.

Since these are only used in one file for now, we can keep the includes in the cpp files.

Comment 38

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to require a return after calls to annotated functions

MozReview-Commit-ID: 7NqXap8FdSn

Comment 39

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

Can you look at this patch now that you've r+-ed the analysis itself :)

MozReview-Commit-ID: LLZPoRrCqvc

Comment 40

[(no longer active)]

Comment on attachment 8836182 [details] [diff] [review]
Part 2: Annotate ErrorResult::Throw as MOZ_MUST_RETURN_FROM_CALLER

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

r=me on the general parts, but I meant to ask you to split this up and ask for review from people who know the code closely because these are behavior changes so it's hard to rubberstamp them...

Comment 41

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add MOZ_MAY_CALL_AFTER_MUST_RETURN and MOZ_MUST_RETURN_FROM_CALLER annotations

MozReview-Commit-ID: 1o2egvdhkqT

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Return early after throwing an error in PeerConnectionImpl::Initialize

MozReview-Commit-ID: AU2V0dtmcLO

Comment 43

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Return early after throwing in AccessibleNode::Get

MozReview-Commit-ID: GmzIhdwVjPj

Comment 44

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Mark some Throw calls in /dom/bindings as having custom cleanup

MozReview-Commit-ID: CooKyfkMlWq

Comment 45

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Mark a throw in FlyWeb as requiring custom cleanup

MozReview-Commit-ID: LwAyet5qCw4

Comment 46

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Handle custom cleanup after throwing in /dom/workers

MozReview-Commit-ID: 1hKUDi9Oxg7

Comment 47

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Avoid doing work after throwing in XMLHttpRequestWorker::SendInternal

MozReview-Commit-ID: BgyhRAJLL81

Comment 48

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Return after ErrorResult::Throw in /dom

These are the remaining fairly simple cases which I think should be much easier to rubber stamp which are found in /dom.

MozReview-Commit-ID: CWjx4L8LTr9

Comment 49

[Andrea Marchesini [:baku]]

Comment on attachment 8842136 [details] [diff] [review]
Part 7: Handle custom cleanup after throwing in /dom/workers

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

::: dom/workers/ScriptLoader.cpp
@@ +2027,5 @@
>      //    sure...
>      if (mScriptLoader.mRv.Failed()) {
>        if (aMutedError && mScriptLoader.mRv.IsJSException()) {
>          LogExceptionToConsole(aCx, aWorkerPrivate);
> +        mScriptLoader.mRv.ThrowWithCustomCleanup(NS_ERROR_DOM_NETWORK_ERR);

why is this? I don't find it in DXR.

Comment 50

[Andrea Marchesini [:baku]]

Comment on attachment 8842137 [details] [diff] [review]
Part 8: Avoid doing work after throwing in XMLHttpRequestWorker::SendInternal

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

::: dom/xhr/XMLHttpRequestWorker.cpp
@@ +1830,5 @@
>  
>    autoUnpin.Clear();
>  
> +  bool succeeded = autoSyncLoop->Run();
> +  mStateData.mFlagSend = false;

Is there a bug in this? Here you are changing the logic. Tell me more.

Comment 51

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrea Marchesini [:baku] from comment #49)
> Comment on attachment 8842136 [details] [diff] [review]
> Part 7: Handle custom cleanup after throwing in /dom/workers
> 
> Review of attachment 8842136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ScriptLoader.cpp
> @@ +2027,5 @@
> >      //    sure...
> >      if (mScriptLoader.mRv.Failed()) {
> >        if (aMutedError && mScriptLoader.mRv.IsJSException()) {
> >          LogExceptionToConsole(aCx, aWorkerPrivate);
> > +        mScriptLoader.mRv.ThrowWithCustomCleanup(NS_ERROR_DOM_NETWORK_ERR);
> 
> why is this? I don't find it in DXR.

ThrowWithCustomCleanup is a new method which is added in another part of this analysis. The `Throw` method has been annotated with `MOZ_MUST_RETURN_FROM_CALLER` which means that the callsite must immediately return after calling the method. ThrowWithCustomCleanup does exactly the same thing as Throw, but doesn't have the annotation, meaning that the caller can do custom cleanup work before returning after calling the throw method.

(In reply to Andrea Marchesini [:baku] from comment #50)
> Comment on attachment 8842137 [details] [diff] [review]
> Part 8: Avoid doing work after throwing in XMLHttpRequestWorker::SendInternal
> 
> Review of attachment 8842137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/xhr/XMLHttpRequestWorker.cpp
> @@ +1830,5 @@
> >  
> >    autoUnpin.Clear();
> >  
> > +  bool succeeded = autoSyncLoop->Run();
> > +  mStateData.mFlagSend = false;
> 
> Is there a bug in this? Here you are changing the logic. Tell me more.

I believe that the new version of the function acts exactly the same as the old one. I was simply rearranging the code such that I could move the `Throw` call to be immediately before the return statement.

Comment 52

[(no longer active)]

Comment on attachment 8842130 [details] [diff] [review]
Part 2: Add MOZ_MAY_CALL_AFTER_MUST_RETURN and MOZ_MUST_RETURN_FROM_CALLER annotations

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

::: dom/bindings/ErrorResult.h
@@ +169,5 @@
>      MOZ_ASSERT(NS_FAILED(rv), "Please don't try throwing success");
>      AssignErrorCode(rv);
>    }
>  
> +  void ThrowWithCustomCleanup(nsresult rv) {

Please document under which circumstances this needs to be called, ideally with a few examples.

Comment 53

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8842139 [details] [diff] [review]
Part 9: Return after ErrorResult::Throw in /dom

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

Ehsan asked me to redirect this review to someone else. qDot, would you feel comfortable looking over these?

Comment 54

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8842137 [details] [diff] [review]
Part 8: Avoid doing work after throwing in XMLHttpRequestWorker::SendInternal

Please see comment 51 for the reply to your questions :).

Comment 55

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8842139 [details] [diff] [review]
Part 9: Return after ErrorResult::Throw in /dom

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

LGTM, though I'm in agreement with Ehsan in Comment 52. Gonna need good documentation of when we want to use CustomCleanup, and then send some sort of all caps email to dev-platform about it or something. Otherwise I think there may be some serious confusion in the future.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f539afe7ee0d
Part 1: Add an analysis to require a return after calls to annotated functions, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/8640e2712eef
Part 2: Add MOZ_MAY_CALL_AFTER_MUST_RETURN and MOZ_MUST_RETURN_FROM_CALLER annotations, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/db7e54e6ee3c
Part 3: Return early after throwing an error in PeerConnectionImpl::Initialize, r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/848c1217afb6
Part 4: Return early after throwing in AccessibleNode::Get, r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/4721910b49ec
Part 5: Mark some Throw calls in /dom/bindings as having custom cleanup, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e07adcfe3f
Part 6: Mark a throw in FlyWeb as requiring custom cleanup, r=djvj
https://hg.mozilla.org/integration/mozilla-inbound/rev/12483600aad9
Part 7: Handle custom cleanup after throwing in /dom/workers, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f7540a0efe
Part 8: Avoid doing work after throwing in XMLHttpRequestWorker::SendInternal, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/17ef6d33babe
Part 9: Return after ErrorResult::Throw in /dom, r=ehsan

Comment 57

[(no longer active)]

Fantastic!

I think it may be a good idea to write about this to dev-platform.  Especially if people have other error prone functions that can use this annotation.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f539afe7ee0d
https://hg.mozilla.org/mozilla-central/rev/8640e2712eef
https://hg.mozilla.org/mozilla-central/rev/db7e54e6ee3c
https://hg.mozilla.org/mozilla-central/rev/848c1217afb6
https://hg.mozilla.org/mozilla-central/rev/4721910b49ec
https://hg.mozilla.org/mozilla-central/rev/89e07adcfe3f
https://hg.mozilla.org/mozilla-central/rev/12483600aad9
https://hg.mozilla.org/mozilla-central/rev/79f7540a0efe
https://hg.mozilla.org/mozilla-central/rev/17ef6d33babe