Closed
Bug 592017
Opened 14 years ago
Closed 2 years ago
Add jsvals to nsIFrameMessageManager.idl
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: azakai, Assigned: smaug)
References
Details
Attachments
(3 files)
1.51 KB,
patch
|
Details | Diff | Splinter Review | |
8.74 KB,
patch
|
Details | Diff | Splinter Review | |
16.09 KB,
patch
|
Details | Diff | Splinter Review |
That IDL currently doesn't use jsvals, since they didn't exist when it was written. If we add jsvals there, it will let those interfaces be called in a more normal manner, including directly from JavaScript. The attachment shows what we want to have, but doesn't work yet, smaug says something in the messagemanager code needs rewriting.
Assignee | ||
Comment 2•14 years ago
|
||
Actually, I was thinking something else - I misunderstood this a bit. So I'm not quite sure what needs to be changed. But I'm looking at this.
Assignee | ||
Comment 3•14 years ago
|
||
So is there are *minimal* testcase for this? Since the simple cases do work with the patch. Alon, you had tested this all with several large-ish patches, but perhaps you have something simpler to test. (Hopefully something which doesn't require Fennec)
Reporter | ||
Comment 4•14 years ago
|
||
Reporter | ||
Comment 5•14 years ago
|
||
Simple testcase. No Fennec needed. 1. Apply the two testcase patches. 2. In the objdir/dist/bin, run make SOLO_FILE="test_contentPrefs_parentipc.js" -C ../../toolkit/components/contentprefs/tests check-one It will crash using a QI to nsIFrameMessageListener and calling receiveMessage, but it will work using the wrappedJSObject hack. To switch between the two, see lines 10-11 in test_contentPrefs_parentipc.js.
Assignee | ||
Comment 6•14 years ago
|
||
TEST-UNEXPECTED-FAIL | runxpcshelltests.py | No tests run. Did you pass an invalid --test-path? INFO | Result summary: INFO | Passed: 0 INFO | Failed: 1 make: *** [check-one] Error 1 make: Leaving directory `/home/smaug/mozilla/hg/puppet/ff_build/toolkit/components/contentprefs/tests'
Assignee | ||
Comment 7•14 years ago
|
||
Ok, I had to comment out ifdef MOZ_PHOENIX
Assignee | ||
Comment 8•14 years ago
|
||
er, oops, my mistake :)
Assignee | ||
Updated•14 years ago
|
Assignee: Olli.Pettay → nobody
Component: IPC → XPConnect
QA Contact: ipc → xpconnect
Assignee | ||
Comment 9•14 years ago
|
||
0x0163a4a6 in XPCConvert::JSData2Native (ccx=..., d=0xb4dfe1b8, s=..., type=..., useAllocator=0, iid=0xbfffdd90, pErr=0x0) at /home/smaug/mozilla/hg/puppet/js/src/xpconnect/src/xpcconvert.cpp:639 #1 0x0164f8fc in nsXPCWrappedJSClass::CallMethod (this=0x82c5840, wrapper=0x82c5948, methodIndex=3, info=0x8161310, nativeParams= 0xbfffdec4) at /home/smaug/mozilla/hg/puppet/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1782 #2 0x01648e92 in nsXPCWrappedJS::CallMethod (this=0x82c5948, methodIndex=3, info=0x8161310, params=0xbfffdec4) at /home/smaug/mozilla/hg/puppet/js/src/xpconnect/src/xpcwrappedjs.cpp:571 #3 0x01b97f86 in PrepareAndDispatch (methodIndex=<value optimized out>, self=0x82c5988, args=0xbfffdf02) at /home/smaug/mozilla/hg/puppet/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95 #4 0x01b972c7 in NS_InvokeByIndex_P () from /home/smaug/mozilla/hg/puppet/ff_build/dist/bin/libxul.so #5 0x01659022 in CallMethodHelper::Call() () from /home/smaug/mozilla/hg/puppet/ff_build/dist/bin/libxul.so #6 0x016533df in XPCWrappedNative::CallMethod (ccx=..., mode=23459421) at /home/smaug/mozilla/hg/puppet/js/src/xpconnect/src/xpcwrappednative.cpp:2304
Assignee | ||
Comment 10•14 years ago
|
||
Alon, I think we may need a non e10s testcase. Jorendorf, I'm not at all familiar with xpconnect's jsval handling.
Comment 11•14 years ago
|
||
case nsXPTType::T_JSVAL : { if (useAllocator) { // The C++ type is (const jsval &), which here means (jsval *). jsval *buf = new jsval(s); if(!buf) return JS_FALSE; *((jsval**)d) = buf; } else { *((jsval*)d) = s; <-- CRASHING HERE } break; } That code looks wrong to me. This might be easy to fix but we still need a small test case. smaug is helping me look for the best way to do that.
Comment 12•14 years ago
|
||
Is this crashing on tracemonkey too? Because the "CRASHING HERE" line from comment 11 is only on tracemonkey, not on mozilla-central, and it was supposed to fix a very similar crash (bug 589329).
Assignee | ||
Comment 13•14 years ago
|
||
bug 589329 doesn't seem to help here.
Reporter | ||
Comment 14•14 years ago
|
||
(In reply to comment #10) > Alon, I think we may need a non e10s testcase. > I don't think e10s is the issue here. With the wrappedJSObject hack it works fine in the test case patches. Furthermore the crash happens in the parent code - at that point, it is still doing exactly the same as a non-e10s xpcshell test (only later does it try to run something in a child process, but it never gets there). So the issue is only the idl/jsval stuff I think.
Assignee | ||
Comment 15•14 years ago
|
||
Yeah, that is true, but a minimal testcase would be great for this.
Comment 16•14 years ago
|
||
moving this out. it isn't a blocker for our betas, but we should clean this up.
tracking-fennec: 2.0b1+ → 2.0+
Comment 17•14 years ago
|
||
Now or post-2.0, we won't take the interface change after the feature freeze.
Updated•14 years ago
|
tracking-fennec: 2.0+ → 2.0b3+
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → Olli.Pettay
Updated•14 years ago
|
blocking2.0: --- → beta7+
Assignee | ||
Comment 18•14 years ago
|
||
So why does this block?
Comment 19•14 years ago
|
||
Yes, I'm confused as to why this should block. bsmedberg, did you make this blocking to force a decision to include this API change?
Comment 20•14 years ago
|
||
This was marked blocking fennec 2.0b3+. Because it contains an API change which will likely affect extensions, it has to land before the API freeze.
Assignee | ||
Comment 21•14 years ago
|
||
But why does this block Fennec? Note, I'll be in W3C TPAC next week, so will have less time for coding.
Assignee | ||
Comment 22•14 years ago
|
||
In other words, this bug blocks non-blocker bug 593407, but is this needed for any blocker bug?
Comment 23•14 years ago
|
||
I have no clue, dougt marked it.
Comment 24•14 years ago
|
||
Adding a whiteboard comment; dougt, is this a release blocker, and if so does it require the API change?
Whiteboard: [doesn't block]
Comment 25•14 years ago
|
||
My understanding is that we would change the message manager recv method to allow for jsvals. This would be an API change that we'd want for b3 to allow addon developers a chance to move to it.
Comment 26•14 years ago
|
||
Can that be done with an incremental API, or must we change the existing one?
Whiteboard: [doesn't block]
Comment 27•14 years ago
|
||
minusing. spoke w/ olli. We agreed that this is a clean up bug. This bug is requesting a change to make it much cleaner for addon developers using the message manager. This change can be done in the future and have devs migrate - and if fact, the change can be backwards compat. I would love to see this bug fixed, but it doesn't block the fennec release.
blocking2.0: beta7+ → -
tracking-fennec: 2.0b3+ → 2.0-
Updated•13 years ago
|
Whiteboard: [fennec-4.1?]
Updated•13 years ago
|
Whiteboard: [fennec-4.1?]
Comment 28•13 years ago
|
||
Is this on the radar for an upcoming release? Adding window.crypto to e10s apps would be so much easier. In bug 673432 - we need to implement window.crypto.getRandomValues for fennec/e10s. getRandomValues is a sync API and expects a typedArray (and is non-allocating). I am not sure how to do that using the messageManager if I am restricted to JSON / strings/ ints.
Comment 29•13 years ago
|
||
As far as I can tell the crash is gone, but I'm seeing bug 655878 in action now.
Depends on: 655878
Assignee | ||
Comment 30•13 years ago
|
||
This bug is not about changing serialization format from JSON to anything else, so I don't understand how this could help with window.crypto. IIRC bsmedberg has plans to change serialization from JSON to structured clones.
Comment 31•12 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #29) > As far as I can tell the crash is gone, but I'm seeing bug 655878 in action > now. With that bug fixed, can this be landed now?
Assignee | ||
Comment 32•2 years ago
|
||
MessageManagers are going away.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•