Closed Bug 1347117 Opened 3 years ago Closed 3 years ago

IsContentParent() assertion failed when trying to focus on a nested remote frame.

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jj.evelyn, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

I'm trying out the patches on bug 558184 which is a nested content process setup, i.e.
Chrome -> content process -> jsplugin process
      \_________________________/

However, my tab crashed when mouse clicking on the pdf content. It is because in the content process the nsFoucusManager tries to call:

        if (TabParent* remote = TabParent::GetFrom(aContent)) {
          remote->Activate();

but in TabParent it assumes that it's under ContentParent's control, however, in our case it's under ContentBridgeParent's control. Therefore the assertion fails in:

        ContentParent* nsIContentParent::AsContentParent()
        {
          MOZ_ASSERT(IsContentParent());
          return static_cast<ContentParent*>(this);
        }

In bug 1334572 we moved Activate(and another two methods) from PBrowser to PContent, and introduced the logic above. I'm wondering how it supposes to work, and, should we add these methods to PContentBridge to resolve the nested case?

:billm, what do you think?
Flags: needinfo?(wmccloskey)
Attached patch wip.patch (obsolete) — Splinter Review
This wip added Activate/Deactivate/ParentActivated to PContentBridge to fix this problem. Tested and it works in our case.
Blocks: 1264551
No longer blocks: 1337537
Depends on: jsplugins-base
Comment on attachment 8847079 [details] [diff] [review]
wip.patch

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

This looks fine to me. Sorry I broke it.

::: browser/extensions/mortar/host/pdf/bootstrap.js
@@ +50,5 @@
>        name: "PPAPI PDF plugin",
>        niceName: "PPAPI PDF plugin",
>        version: "1.0",
>        sandboxScript : `(${sandboxScript.toSource()})(this);`,
> +      ppapiProcessArgs: [ '-rpclib', rpclib, '-pluginlib', pluginlib ],

I don't think this should be in this patch.
Attachment #8847079 - Flags: review+
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #2)
> Comment on attachment 8847079 [details] [diff] [review]
> wip.patch
> 
> Review of attachment 8847079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me. Sorry I broke it.
> 
> ::: browser/extensions/mortar/host/pdf/bootstrap.js
> @@ +50,5 @@
> >        name: "PPAPI PDF plugin",
> >        niceName: "PPAPI PDF plugin",
> >        version: "1.0",
> >        sandboxScript : `(${sandboxScript.toSource()})(this);`,
> > +      ppapiProcessArgs: [ '-rpclib', rpclib, '-pluginlib', pluginlib ],
> 
> I don't think this should be in this patch.

Oops! Right, it shouldn't. 

Thanks for the review. I will add commit messages and push try.
carry over r+ from billm
Attachment #8847079 - Attachment is obsolete: true
Attachment #8847511 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a600f0ea3fb
Add Activate/Deactivate/ParentActivated to PContentBridge; r=billm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1a600f0ea3fb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.