1074211 - Make IPDL call FatalError when AllocPFoo fails

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

This would have make bug 1063658 easier to diagnose at the start, instead of only seeing a generic deserialization error. We'll want to add a message parameter to failIfNullActor and pass it from ctorPrologue (both in http://mxr.mozilla.org/mozilla-central/source/ipc/ipdl/ipdl/lower.py), and if the argument exists we should use it to call _fatalError.

Comment 1

[Josh Matthews [:jdm]]

This can be verified by inspecting the generated output (such as objdir/ipc/ipdl/PContentParent.cpp) - before the change, we see code like
>            actor = AllocPBrowserParent(context, chromeFlags, id, isForApp, isForBrowser);
>            if ((!(actor))) {
>                return MsgValueError;
>            }
and afterwards there should be a FatalError call before the return, like all of the other MsgValue return cases in the file.

Comment 2

[Akshendra Pratap Singh]

I would like to work on this.

Comment 3

[Josh Matthews [:jdm]]

Great! Please ask questions if anything in my explanation is unclear.

Comment 4

[Akshendra Pratap Singh]

Under platform it says x86 Mac OSX, does this bug applies only to OSX

Comment 5

[Josh Matthews [:jdm]]

No; when anyone files a bug it defaults to the platform of the bug reporter.

Comment 6

[Akshendra Pratap Singh]

Attachment: Used message passing from ctroPrologue to failIfNullActor, to add fatalError when the message para is availabel

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 8503637 [details] [diff] [review]
Used message passing from ctroPrologue to failIfNullActor, to add fatalError when the message para is availabel

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

This is looking good so far!

::: ipc/ipdl/ipdl/lower.py
@@ +4895,5 @@
>              idexpr = ExprCall(self.protocol.registerIDMethod(),
>                                args=[ actorvar, idexpr ])
>  
>          return [
> +            self.failIfNullActor(actorvar, errfn, msg="Error AllocPFoo failed"),

"Error constructing actor %s" % self.clsname

I think that should give us an error message that properly names each actor being constructed.

@@ +5133,2 @@
>          failif = StmtIf(ExprNot(actorExpr))
> +        if (msg is not None):

if msg:

Comment 8

[Akshendra Pratap Singh]

There seems to be a problem when I apply this patch, the browser opens up with "Tab crashed" message. And I this this error message has something to do with that
> IPDL protocol error: Error AllocPFoo failed
> [Child 26889] ###!!! ABORT: IPDL error [PContentChild]: "Error AllocPFoo failed". abort()ing as a
> result.: file /home/akshendra/Documents/mozilla-central/ipc/glue/ProtocolUtils.cpp, line 213

Comment 9

[Josh Matthews [:jdm]]

Ah, my mistake. I guess we don't actually want fatalError, since apparently some constructors return null and that's ok. Try switching it to _printWarningMessage instead.

Comment 10

[Akshendra Pratap Singh]

Attachment: Change to _printWarningMessage and added class name in message

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 8504774 [details] [diff] [review]
Change to _printWarningMessage and added class name in message

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

Almost there! Please add a commit message like "Bug 1074211 - Summary of the changes. r=jdm", too. If you're using mq (ie. hg qnew, etc.) you can use hg qref -e to edit the message.

Cheers,
Josh

::: ipc/ipdl/ipdl/lower.py
@@ +4895,5 @@
>              idexpr = ExprCall(self.protocol.registerIDMethod(),
>                                args=[ actorvar, idexpr ])
>  
>          return [
> +            self.failIfNullActor(actorvar, errfn, msg="Error constructing actor %s"% self.clsname),

nit: space before %

@@ +5128,5 @@
>  
>  
>      # helper methods
>  
> +    def failIfNullActor(self, actorExpr, retOnNull=ExprLiteral.FALSE, msg = None):

nit: no need for spaces around = here

@@ +5133,2 @@
>          failif = StmtIf(ExprNot(actorExpr))
> +        if (msg is not None):

if msg:

Comment 12

[Akshendra Pratap Singh]

Attachment: Added commit message and rectified the whitespaces

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8504821 [details] [diff] [review]
Added commit message and rectified the whitespaces

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

This is fine with me, although the commit message is a bit too detailed. Something like "Output a warning when an IPDL constructor returns null." would be more useful for someone reading the commit log.

Comment 14

[Akshendra Pratap Singh]

Should I change that?

Comment 15

[Josh Matthews [:jdm]]

Yes please. You can request review from :bent, so who is the owner of this code.

Comment 16

[Akshendra Pratap Singh]

And do I have to change the name of the reviewer in the commit message?

Comment 17

[Josh Matthews [:jdm]]

Oh, yes. Good catch.

Comment 18

[Akshendra Pratap Singh]

Attachment: 1074211.patch

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8504849 [details] [diff] [review]
1074211.patch

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

Hi Akshendra,

This looks pretty good to me, but I'd just like to verify the changes that get generated. Would you mind attaching a diff of a single generated cpp file that shows change your patch makes?

Thanks!

Comment 20

[Akshendra Pratap Singh]

Attachment: changes.diff

Comment 21

[Akshendra Pratap Singh]

Comment on attachment 8505613 [details]
changes.diff

Here is the diff between the two generated files before and after the patch.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8504849 [details] [diff] [review]
1074211.patch

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

This looks mostly ok but I wonder if we can't make it better...

We have lots of places where we return null from a constructor in the parent in order to force-kill a child process. Those should not be marked as a FatalError. However, I don't think we have any cases where a child process uses this mechanism to kill itself, so marking failed constructors on the child side *could* be marked as FatalError I think.

Would you like to experiment with that approach?

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8504849 [details] [diff] [review]
1074211.patch

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

Actually, I'm sorry, I looked too quickly.

All of your warning messages in the generated file say 'PContentPartent' when the failed constructor is most likely another type.

Comment 24

[Akshendra Pratap Singh]

Attachment: 1074211.patch

Comment 25

[Akshendra Pratap Singh]

Attachment: rectChanges.diff

The new diff file

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8505747 [details] [diff] [review]
1074211.patch

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

Is there a way to get the 'Child' or 'Parent' suffix there too?

Comment 27

[Akshendra Pratap Singh]

I couldn't find one. 

Actually, at other places it was not there, like
> actor = static_cast<PAsmJSCacheEntryParent*>((kids[i])->CloneProtocol((&(mChannel)), aCtx));
> if ((actor) == (nullptr))
> NS_RUNTIMEABORT("can not clone an PAsmJSCacheEntry               
>   return;
> }

So I didn't try much.

Comment 28

[Josh Matthews [:jdm]]

You might be able to use self.side; I would expect all of the error messages to refer to protocols on the same side as the encompassing actor.

Comment 29

[Akshendra Pratap Singh]

I thought about using that but then I didn't because other places also does not denote parent or child(comment #27) and the first character is small letter c or p. So I was not sure.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The full name must be available... It's part of the line declaring the 'actor' argument.

Comment 31

[Akshendra Pratap Singh]

So I will use capitalize() with self.size and then append that to actortype.name()

Comment 32

[Akshendra Pratap Singh]

Attachment: rectChanges2.diff

With parent..

Comment 33

[Akshendra Pratap Singh]

Attachment: 1074211.patch

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8505803 [details] [diff] [review]
1074211.patch

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

This looks great, thanks!

Comment 35

[Akshendra Pratap Singh]

Hey Josh,
What do I need to change in my patch? Is there something wrong?

Comment 36

[Josh Matthews [:jdm]]

No, but thank you for checking! This is ready to be committed; I'll do that later today.

Comment 37

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff587b19c241

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff587b19c241