Closed
Bug 1041419
Opened 10 years ago
Closed 10 years ago
Nested oop browser frame crashes when trying to play a video
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: kershaw, Assigned: kershaw)
References
Details
(Whiteboard: [nested-oop][FT:Stream3])
Attachments
(2 files, 6 obsolete files)
2.91 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
9.52 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Blocks: nested-oop
Updated•10 years ago
|
Whiteboard: [nested-oop][FT:Stream3]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kechang
Assignee | ||
Comment 1•10 years ago
|
||
Hi Kan-Ru, Could you help to review this proposed patch? Thanks.
Attachment #8470604 -
Flags: review?(kchen)
Comment 2•10 years ago
|
||
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.
Attachment #8470604 -
Flags: review?(kchen) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
V2 Patch that includes the feedback above. Please help to review it. Thanks.
Attachment #8470604 -
Attachment is obsolete: true
Attachment #8471314 -
Flags: review?(kchen)
Comment 6•10 years ago
|
||
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.
Attachment #8471314 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8472148 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•10 years ago
|
Attachment #8472150 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8472148 -
Flags: review?(justin.lebar+bug) → review?(bugs)
Updated•10 years ago
|
Attachment #8472148 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Hi Honza, Sorry for bothering. It's been a while without any feedback. Could you please take a look at this patch? Thanks.
![]() |
||
Updated•10 years ago
|
Attachment #8472150 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Rebase and carry reviewer's name
Attachment #8472148 -
Attachment is obsolete: true
Attachment #8511946 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Rebase and carry reviewer's name
Attachment #8472150 -
Attachment is obsolete: true
Attachment #8511947 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•10 years ago
|
||
Cancel checkin due to bug 1020172 has been backed out.
Keywords: checkin-needed
Assignee | ||
Comment 15•10 years ago
|
||
rebase
Attachment #8511946 -
Attachment is obsolete: true
Attachment #8516611 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
rebase
Attachment #8511947 -
Attachment is obsolete: true
Attachment #8516613 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Hi Kershaw, could you provide a try link for this changes? Thanks!
Keywords: checkin-needed
Assignee | ||
Comment 18•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a01a003e43 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e434f4b9d9d
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53a01a003e43 https://hg.mozilla.org/mozilla-central/rev/4e434f4b9d9d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•