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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

({dev-doc-needed})

unspecified
mozilla53
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

6.71 KB, text/x-c++src
Details
1.61 MB, patch
kanru
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

2 years ago
Assignee: nobody → kchen
(Assignee)

Comment 2

2 years ago
Created attachment 8808565 [details] [diff] [review]
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.
Attachment #8808565 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

2 years ago
Created attachment 8808566 [details] [diff] [review]
Part 2. Move GetProtocolTypeId() to IProtocol.

This method can finally be moved to where it belongs.
Attachment #8808566 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

2 years ago
Created attachment 8808567 [details] [diff] [review]
Part 3. Remove leftover CloneProtocol from Nuwa.

Drive by cleanup.
Attachment #8808567 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

2 years ago
Created attachment 8808568 [details] [diff] [review]
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.
Attachment #8808568 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

2 years ago
Created attachment 8808569 [details] [diff] [review]
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.
Attachment #8808569 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

2 years ago
Created attachment 8808570 [details] [diff] [review]
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.
Attachment #8808570 - Flags: review?(wmccloskey)
(Assignee)

Comment 8

2 years ago
Created attachment 8808572 [details]
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)
Setting dev-doc-needed; this is going to need some (probably minor) updates to https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Tutorial
Keywords: dev-doc-needed
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.
(Assignee)

Comment 12

2 years ago
(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.
(Assignee)

Comment 13

2 years ago
Looks like all other changes are already in inbound. I just need to update the main patch.
(Assignee)

Updated

2 years ago
Attachment #8808565 - Attachment is obsolete: true
Attachment #8808565 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8808566 - Attachment is obsolete: true
Attachment #8808566 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8808567 - Attachment is obsolete: true
Attachment #8808567 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8808568 - Attachment is obsolete: true
Attachment #8808568 - Flags: review?(wmccloskey)
(Assignee)

Comment 14

2 years ago
Created attachment 8808916 [details] [diff] [review]
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.
Attachment #8808569 - Attachment is obsolete: true
Attachment #8808569 - Flags: review?(wmccloskey)
Attachment #8808916 - Flags: review?(wmccloskey)
(Assignee)

Comment 15

2 years ago
Created attachment 8808917 [details] [diff] [review]
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.
Attachment #8808570 - Attachment is obsolete: true
Attachment #8808570 - Flags: review?(wmccloskey)
Attachment #8808917 - Flags: review?(wmccloskey)
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.
Attachment #8808916 - Flags: review?(wmccloskey)
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.
Attachment #8808917 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
Created attachment 8809275 [details] [diff] [review]
Add mozilla::ipc::Result type
Attachment #8808916 - Attachment is obsolete: true
Attachment #8809275 - Flags: review?(wmccloskey)
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.
Attachment #8809275 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 22

2 years ago
Created attachment 8810813 [details] [diff] [review]
Add mozilla::ipc::IPCResult type and convert IPDL handlers to use new return type

rebase to autoland
Attachment #8808917 - Attachment is obsolete: true
Attachment #8809275 - Attachment is obsolete: true
Attachment #8810813 - Flags: review+

Comment 23

2 years ago
Pushed by cbook@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39ac4382a2c0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
Depends on: 1318971
Depends on: 1319271
You need to log in before you can comment on or make changes to this bug.