1041419 - Nested oop browser frame crashes when trying to play a video

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Kershaw Chang [:kershaw]]

Steps to reproduce:
1. Follow the steps in Bug 812403 Comment 3 and Bug 812403 Comment 4 to enable nested oop.
2. Open a browser tab and access a youtube page.
3. Browser tab crashes.

Some error messages found in logcat:
/Gecko   (  308): IPDL protocol error: could not look up PBrowser
I/Gecko   (  308): IPDL protocol error: Error deserializing 'PBrowserParent'
I/Gecko   (  308): [Parent 308] ###!!! ASSERTION: IPDL error [PWyciwygChannelParent]: "Error deserializing 'PBrowserParent'". Killing child side as a result.: 'Error', file ../../../gecko/ipc/glue/ProtocolUtils.cpp, line 193
I/Gecko   (  308): 
I/Gecko   (  308): ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Let WyciwygChannel support nested oop iframe - V1

Hi Kan-Ru,

Could you help to review this proposed patch?

Thanks.

Comment 2

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

Comment on attachment 8470604 [details] [diff] [review]
Let WyciwygChannel support nested oop iframe - V1

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

PBrowserOrId probably belongs to dom/ipc/PBrowser.ipdl

::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
@@ +598,5 @@
> +GetBrowserOrId(TabChild* tabChild, WyciwygChannelChild *channel)
> +{
> +  PBrowserOrId browser;
> +  if (!tabChild ||
> +      static_cast<ContentChild*>(channel->Manager()->Manager()) == tabChild->Manager()) {

While you are at this, could you put something like this function in ContentChild and use it in HttpChannelChild too?

ContentChild::GetBrowserOrId(TabChild* aTabChild)
{
  if (!aTabChild ||
      this == aTabChild->Manager()) {
    return aTabChild;
  } else {
    return TabChild::GetTabChildId(aTabChild);
  }
}

@@ +637,5 @@
>    mozilla::dom::TabChild* tabChild = GetTabChild(this);
>    if (MissingRequiredTabChild(tabChild, "wyciwyg")) {
>      return NS_ERROR_ILLEGAL_VALUE;
>    }
> +  PBrowserOrId browser = GetBrowserOrId(tabChild, this);

The MissingRequiredTabChild() check is already failed so we won't have a change to get the Id.

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Kan-Ru Chen [:kanru] from comment #2)
> Comment on attachment 8470604 [details] [diff] [review]
> Let WyciwygChannel support nested oop iframe - V1
> 
> Review of attachment 8470604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> PBrowserOrId probably belongs to dom/ipc/PBrowser.ipdl
> 
It seems that the only way to use PBrowserOrId in PNecko.ipdl and PWyciwygChannel.ipdl is define this in a seperated ipdlh. I've tried to move PBrowserOrIdType.ipdlh to dom/ipc, but ipdl code generator still put PBrowserOrId in mozilla::net.
Currently, I have no idea to do this. Could you provide some tips? 

Thanks.

Comment 4

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

(In reply to Kershaw Chang [:kershaw] from comment #3)
> (In reply to Kan-Ru Chen [:kanru] from comment #2)
> > Comment on attachment 8470604 [details] [diff] [review]
> > Let WyciwygChannel support nested oop iframe - V1
> > 
> > Review of attachment 8470604 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > PBrowserOrId probably belongs to dom/ipc/PBrowser.ipdl
> > 
> It seems that the only way to use PBrowserOrId in PNecko.ipdl and
> PWyciwygChannel.ipdl is define this in a seperated ipdlh. I've tried to move
> PBrowserOrIdType.ipdlh to dom/ipc, but ipdl code generator still put
> PBrowserOrId in mozilla::net.
> Currently, I have no idea to do this. Could you provide some tips? 
> 
> Thanks.

It because you put namespace mozilla { namespace net { ... in the ipdlh. Change that to mozilla::dom should be enough.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Let WyciwygChannel support nested oop iframe - V2

V2 Patch that includes the feedback above.
Please help to review it.

Thanks.

Comment 6

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

Comment on attachment 8471314 [details] [diff] [review]
Let WyciwygChannel support nested oop iframe - V2

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

lgtm. You also need a DOM peer to review the ipc changes and a Necko peer to review the netwerk changes.

::: dom/ipc/ContentChild.cpp
@@ +1900,5 @@
>  
>      return true;
>  }
>  
> +PBrowserOrId*

PBrowserOrId

You don't need to return a pointer here.

@@ +1908,5 @@
> +        this == aTabChild->Manager()) {
> +        return new PBrowserOrId(aTabChild);
> +    }
> +    else {
> +        return new PBrowserOrId(TabChild::GetTabChildId(aTabChild));

return PBrowserOrId(TabChild::GetTabChildId(aTabChild)) and above.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Part0. Add PBrowserOrIdType.ipdlh in mozilla::dom

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Part1. Let WyciwygChannel support nested oop iframe

Comment 9

[Kershaw Chang [:kershaw]]

Hi Justin and Honza,

This bug is a followup of bug 879475 and the proposed patch is actually based on https://bug879475.bugzilla.mozilla.org/attachment.cgi?id=8436782.
Could you help to review this patch?
I think you should be the correct one for reviewing. If not, could you please forward to the appropriate one?

Thanks.

Comment 10

[Kershaw Chang [:kershaw]]

Hi smaug,

Could you help to review this patch?

Thanks.

Comment 11

[Kershaw Chang [:kershaw]]

Hi Honza,

Sorry for bothering.
It's been a while without any feedback. Could you please take a look at this patch?

Thanks.

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Add GetBrowserOrId in ContentChild, r = smaug

Rebase and carry reviewer's name

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Replace PBrowser with PBrowserOrId in PWyciwygChannel, r=honzab

Rebase and carry reviewer's name

Comment 14

[Kershaw Chang [:kershaw]]

Cancel checkin due to bug 1020172 has been backed out.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Add GetBrowserOrId in ContentChild, r = smaug

rebase

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Replace PBrowser with PBrowserOrId in PWyciwygChannel, r=honzab

rebase

Comment 17

[Carsten Book [:Tomcat]]

Hi Kershaw, could you provide a try link for this changes? Thanks!

Comment 18

[Kershaw Chang [:kershaw]]

Sorry that I forgot to put the try link.

Here is the try run I pushed yesterday:
https://tbpl.mozilla.org/?tree=Try&rev=d66b2435fb19

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a01a003e43
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e434f4b9d9d

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53a01a003e43
https://hg.mozilla.org/mozilla-central/rev/4e434f4b9d9d