1314254 - Change IPDL message handlers return value to mozilla::Result or nsresult or just MOZ_CRASH

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Currently when processing IPC messages, MsgValueError and MsgProcessingError errors are due to false returned from handlers.

These errors lead to KillHard in ContentParent, MOZ_CRASH or NS_RUNTIMEABORT in all other top-level actors that implement ProcessingError(). However when we generate the crash report we have lost the context like reasons, line numbers, stacks, etc. IMO we should crash directly or call KillHard when error happens.

I think we can use protocol tree specific method or return value to specialize the behavior. eg.

  // for ContentParent
  class FatalError {
  public:
    FatalError(const char* reason) {
      auto cp = FindContentParent();
      cp->KillHard();
    }
  }
  // default
  class FatalError {
  public:
    FatalError(const char* reason) {
      MOZ_CRASH(reason);
    }
  }

Note: not all top-level protocols implement ProcessingError, which means their errors are ignored silently. I'm not sure if it's intentional or just an oversight of the implementer due to the lack of document. This bug should also make this situation better.

http://searchfox.org/mozilla-central/search?q=%3A%3AProcessingError&case=false&regexp=false&path=

What do you think, Bill?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, sounds great to me. I think that using a special class for the return type makes sense. The constructor for that class would call some sort of protocol-specific virtual function. Maybe like this:

#define IPC_OK() mozilla::ipc::Result::Ok()
#define IPC_FAIL(actor, why) mozilla::ipc::Result::Fail((actor), __FILE__, __LINE__, (why))

ipc::Result RecvFoobar() {
  if (...) {
    return IPC_FAIL(this, "unable to xyz");
  }
  return IPC_OK();
}

namespace ipc {
class Result {
public:
  static Result Ok() { return Result(true); }
  static Result Fail(IProtocol* actor, const char* filename, int line, const char* why) {
    // This would be a virtual call. For ContentParent it could include the
    // filename and line in the crash dump. Perhaps we could even generate a
    // paired minidump of the browser and content process.
    // The default implementation would MOZ_CRASH.
    actor->ReceiveFailure(filename, line, why);
    return Result(false);
  }

private:
  Result(bool);
  bool mSuccess;
}

The special type would enforce the requirement that everyone has to call either Ok or Fail.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Merge IProtocolManager and IProtocol

Preparing to use actor methods from a IProtocol pointer. All actors
inherit from IProtocolManager and IProtocol makes the former redundant.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Move GetProtocolTypeId() to IProtocol.

This method can finally be moved to where it belongs.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 3. Remove leftover CloneProtocol from Nuwa.

Drive by cleanup.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 4. Add mozilla::ipc::StringFromIPCProtocolId

We will use this in the crash messages to add the protocol name to the
signature to make the it cluster better and more readable.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 5.0. Add mozilla::ipc::Result type

The main patch. We will use the new type for the generated IPDL message
handler prototype to make sure correct error handling method is called.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 5.1. Convert IPDL handlers to use new return type

The main patch that converts the existing code to return the correct
type. The patch is mostly generated from a clang source-to-source
transform tool. Some aesthetics changes are applied afterwards.

This patch is intended to be squashed to the main patch and landed as
one commit.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: ipc_rewriter.cpp

Code that automatically rewrite the IPDL return types. First build the compilation databes by |mach build-backend --backend=CompileDB| then run ipc-rewriter on the file you want to transform.

compile with:

CLANG_LIBS := \
        -Wl,--start-group \
        -lclangAST \
        -lclangASTMatchers \
        -lclangAnalysis \
        -lclangBasic \
        -lclangDriver \
        -lclangEdit \
        -lclangFrontend \
        -lclangFrontendTool \
        -lclangLex \
        -lclangParse \
        -lclangSema \
        -lclangEdit \
        -lclangRewrite \
        -lclangRewriteFrontend \
        -lclangStaticAnalyzerFrontend \
        -lclangStaticAnalyzerCheckers \
        -lclangStaticAnalyzerCore \
        -lclangSerialization \
        -lclangToolingCore \
        -lclangTooling \
        -lclangFormat \
        -Wl,--end-group

LLVM_FLAGS := $(shell llvm-config-4.0 --cxxflags --ldflags --libs --system-libs)

all: ipc_rewriter

ipc_rewriter: ipc_rewriter.cpp
        g++ -o ipc_rewriter -std=c++11 ipc_rewriter.cpp $(LLVM_FLAGS) $(CLANG_LIBS)

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

The new crash report looks like this
https://crash-stats.mozilla.com/report/index/1924535e-46e3-4dfd-ab09-015d62161108

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Setting dev-doc-needed; this is going to need some (probably minor) updates to https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Tutorial

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Kan-Ru. This looks nice.

It looks like you and I made some similar changes to IPDL. Can you please try rebasing over bug 792652? I'm hoping it will stick tonight. Sorry for the conflicts.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Bill McCloskey (:billm) from comment #11)
> Thanks Kan-Ru. This looks nice.
> 
> It looks like you and I made some similar changes to IPDL. Can you please
> try rebasing over bug 792652? I'm hoping it will stick tonight. Sorry for
> the conflicts.

No problem. I'm expecting my patch will conflict as hell with other patches.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Looks like all other changes are already in inbound. I just need to update the main patch.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add mozilla::ipc::Result type

The main patch. We will use the new type for the generated IPDL message
handler prototype to make sure correct error handling method is called.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Convert IPDL handlers to use new return type.

The main patch that converts the existing code to return the correct
type. The patch is mostly generated from a clang source-to-source
transform tool. Some aesthetics changes are applied afterwards.

This patch is intended to be squashed to the main patch and landed as
one commit.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8808916 [details] [diff] [review]
Add mozilla::ipc::Result type

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

This looks good, but I think I see a problem.

::: ipc/glue/ProtocolUtils.h
@@ +216,5 @@
> +
> +/**
> + * All message deserializer and message handler should return this
> + * type via above macros. The name may conflict with mozilla::Result
> + * in the future because we have quite some using namespace

s/quite some/quite a few/.

Also, given the potential for name collisions, we should probably pick a less generic name. I hadn't though about that before you brought it up. Maybe IPCResult? I wouldn't want to have to do another big rewrite after this one.

::: ipc/ipdl/ipdl/lower.py
@@ +4590,5 @@
>                                           implicit=implicit))))
>          failif.addifstmts([
>              _protocolErrorBreakpoint('Handler returned error code!'),
> +            Whitespace('// Error handled in mozilla::ipc::Result\n', indent=1),
> +            StmtReturn(_Result.Processed)

This won't work. If we return MsgProcessed, then we won't run this code:
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/ipc/glue/MessageChannel.cpp#1718-1722

I think it would be better to return MsgProcessingError, but to return early here:
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/ipc/glue/MessageChannel.cpp#2068
before calling ProcessingError (again).

I also wonder if we could use the same approach for all MsgXYZ return values. It might be nice if the crash stack included the IPDL generated code for things like MsgPayloadError.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8808917 [details] [diff] [review]
Convert IPDL handlers to use new return type.

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

Thanks so much! How much manual work was involved here? I'm curious how well the rewriting tool works.

::: accessible/ipc/other/DocAccessibleChild.h
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Not sure why, but the diff includes the entire file.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Bill McCloskey (:billm) from comment #16)
> Comment on attachment 8808916 [details] [diff] [review]
> Add mozilla::ipc::Result type
> 
> Review of attachment 8808916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but I think I see a problem.
> 
> ::: ipc/glue/ProtocolUtils.h
> @@ +216,5 @@
> > +
> > +/**
> > + * All message deserializer and message handler should return this
> > + * type via above macros. The name may conflict with mozilla::Result
> > + * in the future because we have quite some using namespace
> 
> s/quite some/quite a few/.
> 
> Also, given the potential for name collisions, we should probably pick a
> less generic name. I hadn't though about that before you brought it up.
> Maybe IPCResult? I wouldn't want to have to do another big rewrite after
> this one.

Yes, IPCResult. I thought about using that one but somehow decided it's less beautiful. Changing that in my patch is pretty straightforward.

> ::: ipc/ipdl/ipdl/lower.py
> @@ +4590,5 @@
> >                                           implicit=implicit))))
> >          failif.addifstmts([
> >              _protocolErrorBreakpoint('Handler returned error code!'),
> > +            Whitespace('// Error handled in mozilla::ipc::Result\n', indent=1),
> > +            StmtReturn(_Result.Processed)
> 
> This won't work. If we return MsgProcessed, then we won't run this code:
> http://searchfox.org/mozilla-central/rev/
> 008fdd93290c83325de6629a1ae48dc2afaed868/ipc/glue/MessageChannel.cpp#1718-
> 1722
> 
> I think it would be better to return MsgProcessingError, but to return early
> here:
> http://searchfox.org/mozilla-central/rev/
> 008fdd93290c83325de6629a1ae48dc2afaed868/ipc/glue/MessageChannel.cpp#2068
> before calling ProcessingError (again).

OK, sounds good.

> I also wonder if we could use the same approach for all MsgXYZ return
> values. It might be nice if the crash stack included the IPDL generated code
> for things like MsgPayloadError.

I would like to use it for MsgValueError but I figured this bug is already too large so I will do that in a followup. I'm not sure about the other MsgXYZ return values, they are usually generated code and tend to be stable.

(In reply to Bill McCloskey (:billm) from comment #17)
> Comment on attachment 8808917 [details] [diff] [review]
> Convert IPDL handlers to use new return type.
> 
> Review of attachment 8808917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks so much! How much manual work was involved here? I'm curious how well
> the rewriting tool works.

The tool is pretty good but could be better. The manual work involved includes adjust indentation for long function call, replace !NS_SUCCEED with NS_FAILED, handle return inside a macro like NS_ENSURE_*

The latter ones could be automated but they are rare enough I just edited them manually.
 
> ::: accessible/ipc/other/DocAccessibleChild.h
> @@ -1,1 @@
> > -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> Not sure why, but the diff includes the entire file.

Probably because the change toughed almost the entire file.

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add mozilla::ipc::Result type

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8809275 [details] [diff] [review]
Add mozilla::ipc::Result type

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

Thanks so much! Sorry for the delay.

::: ipc/glue/MessageChannel.cpp
@@ +2118,5 @@
>      }
>  
>      PrintErrorMessage(mSide, channelName, reason);
>  
> +    // Error handled in mozilla::ipc::IPCResult

Comma at the end of the sentence please.

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add mozilla::ipc::IPCResult type and convert IPDL handlers to use new return type

rebase to autoland

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39ac4382a2c0
Add mozilla::ipc::IPCResult type and convert IPDL handlers to use new return type. r=billm

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39ac4382a2c0