Closed Bug 1074211 Opened 5 years ago Closed 5 years ago

Make IPDL call FatalError when AllocPFoo fails

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jdm, Assigned: akshendra521994, Mentored)

Details

(Whiteboard: [lang=python][good first bug])

Attachments

(2 files, 7 obsolete files)

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.
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.
I would like to work on this.
Great! Please ask questions if anything in my explanation is unclear.
Under platform it says x86 Mac OSX, does this bug applies only to OSX
No; when anyone files a bug it defaults to the platform of the bug reporter.
OS: Mac OS X → All
Hardware: x86 → All
Attachment #8503637 - Attachment description: Used message passing from ctroPrologue to failIfNullActor, to add fatorError when the message para is availabel → Used message passing from ctroPrologue to failIfNullActor, to add fatalError when the message para is availabel
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:
Attachment #8503637 - Flags: review?(josh) → feedback+
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
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.
Attachment #8503637 - Attachment is obsolete: true
Attachment #8504774 - Flags: review?(josh)
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:
Attachment #8504774 - Flags: review?(josh) → feedback+
Attachment #8504774 - Attachment is obsolete: true
Attachment #8504821 - Flags: review?(josh)
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.
Attachment #8504821 - Flags: review?(josh)
Attachment #8504821 - Flags: review?(bent.mozilla)
Attachment #8504821 - Flags: feedback+
Should I change that?
Yes please. You can request review from :bent, so who is the owner of this code.
And do I have to change the name of the reviewer in the commit message?
Oh, yes. Good catch.
Attached patch 1074211.patch (obsolete) — Splinter Review
Attachment #8504821 - Attachment is obsolete: true
Attachment #8504821 - Flags: review?(bent.mozilla)
Attachment #8504849 - Flags: review?(bent.mozilla)
Assignee: nobody → akshendra521994
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!
Attachment #8504849 - Flags: feedback+
Attached file changes.diff (obsolete) —
Comment on attachment 8505613 [details]
changes.diff

Here is the diff between the two generated files before and after the patch.
Attachment #8505613 - Attachment mime type: text/x-patch → text/plain
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?
Attachment #8504849 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8504849 - Flags: review-
Attachment #8504849 - Flags: review+
Attachment #8504849 - Flags: feedback+
Attached patch 1074211.patch (obsolete) — Splinter Review
Attachment #8504849 - Attachment is obsolete: true
Attachment #8505747 - Flags: review?(bent.mozilla)
Attached file rectChanges.diff (obsolete) —
The new diff file
Attachment #8505613 - Attachment is obsolete: true
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?
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.
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.
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.
The full name must be available... It's part of the line declaring the 'actor' argument.
So I will use capitalize() with self.size and then append that to actortype.name()
Attached file rectChanges2.diff
With parent..
Attachment #8505748 - Attachment is obsolete: true
Attached patch 1074211.patchSplinter Review
Attachment #8505747 - Attachment is obsolete: true
Attachment #8505747 - Flags: review?(bent.mozilla)
Attachment #8505803 - Flags: review?(bent.mozilla)
Comment on attachment 8505803 [details] [diff] [review]
1074211.patch

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

This looks great, thanks!
Attachment #8505803 - Flags: review?(bent.mozilla) → review+
Hey Josh,
What do I need to change in my patch? Is there something wrong?
Flags: needinfo?(josh)
No, but thank you for checking! This is ready to be committed; I'll do that later today.
Flags: needinfo?(josh)
https://hg.mozilla.org/mozilla-central/rev/ff587b19c241
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.