592017 - Add jsvals to nsIFrameMessageManager.idl

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Alon Zakai (:azakai)]

Attachment: Patch

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.

Comment 1

[Doug Turner (:dougt)]

this is needed for a b1 blocking bug.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 4

[Alon Zakai (:azakai)]

Attachment: Testcase 1/2

Comment 5

[Alon Zakai (:azakai)]

Attachment: Testcase 2/2

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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'

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ok, I had to comment out ifdef MOZ_PHOENIX

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

er, oops, my mistake :)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Alon, I think we may need a non e10s testcase.

Jorendorf, I'm not at all familiar with xpconnect's jsval handling.

Comment 11

[Jason Orendorff [:jorendorff]]

    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

[Peter Van der Beken [:peterv]]

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).

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

bug 589329 doesn't seem to help here.

Comment 14

[Alon Zakai (:azakai)]

(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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Yeah, that is true, but a minimal testcase would be great for this.

Comment 16

[Doug Turner (:dougt)]

moving this out.  it isn't a blocker for our betas, but we should clean this up.

Comment 17

[Benjamin Smedberg]

Now or post-2.0, we won't take the interface change after the feature freeze.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

So why does this block?

Comment 19

[Damon Sicore (:damons)]

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

[Benjamin Smedberg]

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.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

But why does this block Fennec?
Note, I'll be in W3C TPAC next week, so will have less time for coding.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

In other words, this bug blocks non-blocker bug 593407, but is this needed
for any blocker bug?

Comment 23

[Benjamin Smedberg]

I have no clue, dougt marked it.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Adding a whiteboard comment; dougt, is this a release blocker, and if so does it require the API change?

Comment 25

[Doug Turner (:dougt)]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

Can that be done with an incremental API, or must we change the existing one?

Comment 27

[Doug Turner (:dougt)]

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.

Comment 28

[David Dahl :ddahl]

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

[Josh Matthews [:jdm]]

As far as I can tell the crash is gone, but I'm seeing bug 655878 in action now.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

MessageManagers are going away.