Closed Bug 592017 Opened 12 years ago Closed 3 months ago

Add jsvals to nsIFrameMessageManager.idl


(Core :: XPConnect, defect)

Not set



Tracking Status
blocking2.0 --- -
fennec - ---


(Reporter: azakai, Assigned: smaug)




(3 files)

Attached patch PatchSplinter 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.
this is needed for a b1 blocking bug.
tracking-fennec: --- → 2.0b1+
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.
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)
Attached patch Testcase 1/2Splinter Review
Attached patch Testcase 2/2Splinter Review
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.
TEST-UNEXPECTED-FAIL | | 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'
Ok, I had to comment out ifdef MOZ_PHOENIX
er, oops, my mistake :)
Assignee: Olli.Pettay → nobody
Component: IPC → XPConnect
QA Contact: ipc → xpconnect
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/
#5  0x01659022 in CallMethodHelper::Call() () from /home/smaug/mozilla/hg/puppet/ff_build/dist/bin/
#6  0x016533df in XPCWrappedNative::CallMethod (ccx=..., mode=23459421)
    at /home/smaug/mozilla/hg/puppet/js/src/xpconnect/src/xpcwrappednative.cpp:2304
Alon, I think we may need a non e10s testcase.

Jorendorf, I'm not at all familiar with xpconnect's jsval handling.
    case nsXPTType::T_JSVAL :
            if (useAllocator) {
                // The C++ type is (const jsval &), which here means (jsval *).
                jsval *buf = new jsval(s);
                    return JS_FALSE;
                *((jsval**)d) = buf;
            } else {
                *((jsval*)d) = s;          <-- CRASHING HERE

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.
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).
bug 589329 doesn't seem to help here.
(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.
Yeah, that is true, but a minimal testcase would be great for this.
Blocks: 593407
moving this out.  it isn't a blocker for our betas, but we should clean this up.
tracking-fennec: 2.0b1+ → 2.0+
Now or post-2.0, we won't take the interface change after the feature freeze.
tracking-fennec: 2.0+ → 2.0b3+
Assignee: nobody → Olli.Pettay
blocking2.0: --- → beta7+
So why does this block?
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?
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.
But why does this block Fennec?
Note, I'll be in W3C TPAC next week, so will have less time for coding.
In other words, this bug blocks non-blocker bug 593407, but is this needed
for any blocker bug?
I have no clue, dougt marked it.
Adding a whiteboard comment; dougt, is this a release blocker, and if so does it require the API change?
Whiteboard: [doesn't block]
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.
Can that be done with an incremental API, or must we change the existing one?
Whiteboard: [doesn't block]

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-
Whiteboard: [fennec-4.1?]
Whiteboard: [fennec-4.1?]
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.
Blocks: 673432
As far as I can tell the crash is gone, but I'm seeing bug 655878 in action now.
Depends on: 655878
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.
No longer blocks: 673432
(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?

MessageManagers are going away.

Closed: 3 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.