952927 - Expose raw data on UDP socket messages

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mike de Boer [:mikedeboer]]

Since bug 869869 we have an awesome scriptable API for UDP/ datagram connections. However, as mentioned there in comment 120, `nsIUDPMessage` does not provide access to the raw, binary incoming socket data.
`nsIUDPSocket::send` and `nsIUDPSocket::sendWithAddr` specifically prefer a Uint8Array as argument.

Adding a `nsIUDPMessage::rawData` property of type `Uint8Array` ([array] uint8_t rawData) would resolve this and add symmetry to the API!

Comment 1

[sbetageri111]

I wouldl like to work on this bug. How can I begin?

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi @sbeta, you can start by modifying the IDL in http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIUDPSocket.idl?from=nsIUDPSocket.idl#195 and implement the new attribute/method in http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsUDPSocket.cpp?from=nsUDPSocket.cpp#140.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hmm...our XPIDL doesn't support array type attribute now, so we might need to add a method for it. http://dxr.mozilla.org/mozilla-central/source/storage/public/mozIStorageStatement.idl#209 is a good example for returning array-like value.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> ([array] uint8_t rawData)

That's not a Uint8Array.  The only way to express Uint8Array in xpidl is with "jsval".

Comment 5

[Pranav Kant [:pranavk]]

Hi, I am a newcomer trying to fix this bug. 

(In reply to Shih-Chiang Chien [:schien] from comment #3)
> Hmm...our XPIDL doesn't support array type attribute now, so we might need
> to add a method for it.
> http://dxr.mozilla.org/mozilla-central/source/storage/public/
> mozIStorageStatement.idl#209 is a good example for returning array-like
> value.

There is no rawData available in any of the variable in the nsUDPMessage class (http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsUDPSocket.h#61). So should the new method GetRawData() take ACString as input and convert that into uint8_t array or should the class nsUDPMessage be changed accordingly to add a new rawData variable for direct access to the raw data ?

Sorry if the doubt is too newbie.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I think we should change the type of mData to uint8_t array and convert it to ACString when someone invoke the GetData(). In this way we can implement a GetRawData() that simply output the uint8_t array.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Are you designing this API to be used from C++ or from JS?

If the latter, you should be outputting a Uint8Array, presumably, right?

Comment 8

[Mike de Boer [:mikedeboer]]

> If the latter, you should be outputting a Uint8Array, presumably, right?

That'd be preferred: Uint8Array in, Uint8Array out. In we have, out is what I envisioned this bug would tackle.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The in you sort of have.  It'll accept any typed array or non-typed array, and coerce the elements to uint8_t.  It also requires a separate length to be passed in...

But yes, the out should definitely be returning a Uint8Array.  Though you may want to think about what happens if the caller then writes to that array.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Though you may want to think about what happens if the caller then writes to that array.

noop? I mean, it'd be unexpected if a mutation to incoming data would write it back to the sender, for example. It'd be different if it were a bi-directional stream API, but that's not what this UDPMessage API implies.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> noop?

No, I mean what happens to the objects.  Obviously nothing happens on the UDP level.

Basically, either you do a .rawData that returns a Uint8Array and if the caller writes to it then it mutates the thing .rawData is returning, or you have a .getRawData() method that returns a new Uint8Array each time...

Comment 12

[Mike de Boer [:mikedeboer]]

> Basically, either you do a .rawData that returns a Uint8Array and if the
> caller writes to it then it mutates the thing .rawData is returning.

AH, gotcha! Since the message is an instance of UDPMessage, I think it'd fine to accept .rawData as an instance member and that writes mutate it directly.

Comment 13

[Pranav Kant [:pranavk]]

Attachment: Bug_952927.patch

Require review to my first ever patch to Mozilla. Pardon for any mistakes. Critics are strongly invited to cure me.

Comment 14

[Pranav Kant [:pranavk]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Are you designing this API to be used from C++ or from JS?
> 

I am not changing the interface definition, it was scriptable so means that it was and is meant to be used from both JS and C++. 

> If the latter, you should be outputting a Uint8Array, presumably, right?

I read the XPIDL docs (https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL) and do not find any Uint8Array, however to represent uint8_t *, there is this 'octet', so I used that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8359703 [details] [diff] [review]
Bug_952927.patch

>+    readonly attribute octet rawData;

That returns a single byte.

>+  NS_IF_ADDREF(aRawData = mData);

And this can't possibly compile...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> it was scriptable so means that it was and is meant to be used from both JS and C++. 

That doesn't follow, actually.  Scriptable just means it can be used from JS.  The question is where you _expect_ it to be used from.

> and do not find any Uint8Array

See comment 4.

> however to represent uint8_t *, there is this 'octet'

"octet" represents a uint8_t, not a uint8_t*.  But since it's being _returned_, you pass a pointer to the thing it represents.

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8359703 [details] [diff] [review]
Bug_952927.patch

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

Thanks for the contribution and here is my comment. Looking forward to see your updates.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +211,5 @@
> +
> +     /**
> +     * Raw Data of the message
> +     */
> +    readonly attribute octet rawData;

octet is representing uint8_t in C++, which cannot be the type of uint8 array.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +142,5 @@
>  NS_IMPL_ISUPPORTS1(nsUDPMessage, nsIUDPMessage)
>  
>  nsUDPMessage::nsUDPMessage(PRNetAddr* aAddr,
>               nsIOutputStream* aOutputStream,
>               const nsACString& aData)

I'll expect to modify this parameter and carry uint8 array directly. Using nsTArray<uint8_t> should be a better choice.

@@ +150,5 @@
> +  const char *data;
> +  /* Assumes that length returned always fits in a 64 bit unsigned integer */
> +  mLen = NS_CStringGetData(aData, &data);
> +  mData = new uint8_t[mLen];
> +  

nit: remember to remove all the tailing space.

@@ +154,5 @@
> +  
> +  for(PRUint64 i = 0; i < mLen; ++i)
> +   {
> +    mData[i] = data[i];
> +   }

We can reduce the memory copy by using nsTArray::Swap().

@@ +183,5 @@
> + for(PRUint64 i = 0; i < mLen; ++i)
> +  {
> +   cData[i] = mData[i];
> +  }
> +  NS_CStringSetData(aData, cData);

You could find out a better way to creating nsACString from char/uint8 array, go check https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide?redirectlocale=en-US&redirectslug=Mozilla/XPCOM/Strings#Appendix_A_-_What_class_to_use_when.

@@ +196,5 @@
>    return NS_OK;
>  }
>  
> +/* readonly attribute Uint8Array rawData; */
> +NS_IMETHODIMP nsUDPMessage::GetRawData(uint8_t *aRawData)

This interface only returns uint8 value, not the uint8 array we want.

@@ +199,5 @@
> +/* readonly attribute Uint8Array rawData; */
> +NS_IMETHODIMP nsUDPMessage::GetRawData(uint8_t *aRawData)
> +{
> +  NS_ENSURE_ARG_POINTER(aRawData);
> +  NS_IF_ADDREF(aRawData = mData);

NS_IF_ADDREF is for maintaining reference count of a XPCOM object. We don't need this for passing array pointer.

::: netwerk/base/src/nsUDPSocket.h
@@ +73,5 @@
>  
>    PRNetAddr mAddr;
>    nsCOMPtr<nsIOutputStream> mOutputStream;
> +  uint8_t *mData;
> +  PRUint64 mLen;

should use nsTArray<uint8_t> here.

Comment 18

[Pranav Kant [:pranavk]]

Attachment: b952927.patch

As suggested by you and :bz, I have made few more modification to the code towards fixing this bug. I'd be happy to make more modifications in case of problems with existing patch (which I am quite sure of).

Comment 19

[Pranav Kant [:pranavk]]

And can you please assign this this bug against my name.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pranav, have you tested this patch?

Comment 21

[Pranav Kant [:pranavk]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> Pranav, have you tested this patch?

Not as of now. I ran 'make -f client.mk build' but that did not compile the code successfully, it gave some JSContext related errors.

Comment 22

[Pranav Kant [:pranavk]]

The build failed with following error.

> In file included from /home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:0:
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:4884:1: warning: missing braces around initializer for ‘JSPropertyOpWrapper’ [-Wmissing-braces]
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:4884:1: warning: missing braces around initializer for ‘JSStrictPropertyOpWrapper’ [-Wmissing-braces]
> In file included from /home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:0:
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/jsoptparse.cpp: In function ‘bool Evaluate(JSContext*, unsigned int, jsval*)’:
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/jsoptparse.cpp:626:1: warning: ‘*((void*)(& ancx)+32).js::AutoCompartment::origin_’ may be used uninitialized in this function [-Wuninitialized]
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+32).js::AutoCompartment::origin_’ was declared here
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/../jscntxtinlines.h:433:53: warning: ‘*((void*)(& ancx)+32).js::AutoCompartment::cx_’ may be used uninitialized in this function [-Wuninitialized]
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+32).js::AutoCompartment::cx_’ was declared here
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/../jsapi.h:1360:32: warning: ‘*((void*)(& ancx)+16).JSAutoRequest::mContext’ may be used uninitialized in this function [-Wuninitialized]
> /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+16).JSAutoRequest::mContext’ was declared here
> make[7]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell'
> make[7]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/jsapi-tests'
> make[6]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src'
> make[5]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src'
> make[4]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> make[3]: *** [compile] Error 2
> make[3]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> make[2]: *** [default] Error 2
> make[2]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> make[1]: *** [realbuild] Error 2
> make[1]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central'
> make: *** [build] Error 2

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8360601 [details] [diff] [review]
b952927.patch

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

We are on the right direction but need further modification.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +211,5 @@
> +
> +    /**
> +     * Raw Data of the message
> +     */
> +    readonly attribute jsval rawData;

We also need a new interface for C++ to access raw data.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +147,2 @@
>    : mOutputStream(aOutputStream)
>    , mData(aData)

We can avoid memory copy by using nsTArray::SwapElements() here, see examples in http://dxr.mozilla.org/mozilla-central/source/xpcom/tests/TestTArray.cpp#473.

@@ +173,5 @@
>  {
> +  PRUint32 len = mData.Count();
> +  for(PRUint32 i = 0; i < len; i++){
> +      aData.Append(mData[i]);
> +  }

You can reuse the code from http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsUDPSocket.cpp?from=nsUDPSocket.cpp#337.

@@ +188,5 @@
>  
> +/* readonly attribute jsval rawData; */
> +NS_IMETHODIMP nsUDPMessage::GetRawData(jsval *aRawData)
> +{
> +  aRawData = mData;

This won't compile. You cannot convert nsTArray to jval. See example in http://dxr.mozilla.org/mozilla-central/source/dom/encoding/TextEncoder.cpp#78 to know about how to create Uint8Array.

@@ +343,5 @@
>    }
>    mByteReadCount += count;
>  
> +  nsCString sData;
> +  if (!sData.Assign(buff, count, mozilla::fallible_t())) {

We should be able to construct nsTArray directly from buff, we don't need this nsCString. We should prevent memory copy here, too.

@@ +354,5 @@
> +  PRUint32 len = sData.Length();
> +  for(PRUint32 i = 0; i < len; i++){
> +      data.AppendElement(sData.charAt(i));
> +  }
> +    

nit: tailing space.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Pranav Kant from comment #22)
> The build failed with following error.
> 
> > In file included from /home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:0:
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:4884:1: warning: missing braces around initializer for ‘JSPropertyOpWrapper’ [-Wmissing-braces]
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:4884:1: warning: missing braces around initializer for ‘JSStrictPropertyOpWrapper’ [-Wmissing-braces]
> > In file included from /home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:0:
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/jsoptparse.cpp: In function ‘bool Evaluate(JSContext*, unsigned int, jsval*)’:
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/jsoptparse.cpp:626:1: warning: ‘*((void*)(& ancx)+32).js::AutoCompartment::origin_’ may be used uninitialized in this function [-Wuninitialized]
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+32).js::AutoCompartment::origin_’ was declared here
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/../jscntxtinlines.h:433:53: warning: ‘*((void*)(& ancx)+32).js::AutoCompartment::cx_’ may be used uninitialized in this function [-Wuninitialized]
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+32).js::AutoCompartment::cx_’ was declared here
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/../jsapi.h:1360:32: warning: ‘*((void*)(& ancx)+16).JSAutoRequest::mContext’ may be used uninitialized in this function [-Wuninitialized]
> > /home/pranavk/pro/mozilla/mozilla-central/js/src/../../js/src/shell/js.cpp:997:20: note: ‘*((void*)(& ancx)+16).JSAutoRequest::mContext’ was declared here
> > make[7]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/shell'
> > make[7]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src/js/src/jsapi-tests'
> > make[6]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src'
> > make[5]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/js/src'
> > make[4]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> > make[3]: *** [compile] Error 2
> > make[3]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> > make[2]: *** [default] Error 2
> > make[2]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu'
> > make[1]: *** [realbuild] Error 2
> > make[1]: Leaving directory `/home/pranavk/pro/mozilla/mozilla-central'
> > make: *** [build] Error 2

These are all just warnings. Use |./mach build -j1| to stop at the build error.

Comment 25

[Pranav Kant [:pranavk]]

Attachment: b_952927.patch

The above patch is also checked by me. It is compiling successfully. If there are further modifications needs to be made, I shall be happy to work again on those.

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8362050 [details] [diff] [review]
b_952927.patch

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

I'm not familiar with the Uint8Array handling, @bz could you help review this part?

@pranav, you need to add test case that using the new API to ensure your patch really works. You may extend the netwerk/test/TestUDPSocket.cpp for examining C++ API and create an xpcshell test[1] for examining the Javascript API.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

::: netwerk/base/public/nsIUDPSocket.idl
@@ +213,5 @@
> +     * Raw Data of the message
> +     */
> +    [implicit_jscontext]
> +    readonly attribute jsval rawData;
> +    [noscript, notxpcom, nostdcall] charPtr cGetRawData();

1. What's the reason of declaring "notxpcom" and "nostdcall"?
2. Output a nsTArray is better in my opinion.
3. The function name seems to have a unnecessary "c".

::: netwerk/base/src/nsUDPSocket.cpp
@@ +174,5 @@
>  {
> +  PRUint32 len = mData.Length();
> +  for(PRUint32 i = 0; i < len; i++){
> +      aData.Append(mData[i]);
> +  }

Should be able to use |aData.Assign(mData.Elements(), mData.Length());| here instead of creating a for loop. reference: http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h?from=nsACString#460

@@ +352,5 @@
>    mByteReadCount += count;
>  
> +  nsTArray<uint8_t> data;
> +  for(uint32_t i = 0; i < count; i++){
> +      data.AppendElement(buff[i]);

use |AppendElements| instead of writing a for loop, see example http://dxr.mozilla.org/mozilla-central/source/xpcom/tests/TestTArray.cpp#38

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

BTW, Start bringing @mcmanus to the review process since he is the necko peer.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@pranav, have you applied the level-1 access for submitting auto test request to the try server? If not, please follow the instruction in http://www.mozilla.org/hacking/commit-access-policy/. We need to pass the auto test before committing patches to our repository.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8362050 [details] [diff] [review]
b_952927.patch

Returning a new array every time is pretty suboptimal.  Do you expect code to do loops like so:

  for (var i = 0; i < foo.rawData.length; ++i) {
    doSomething(foo.rawData[i]);
  }

?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> 1. What's the reason of declaring "notxpcom" and "nostdcall"?

To get a sane C++ function signature.

> 3. The function name seems to have a unnecessary "c".

You can't have both a "rawData" attribute and a "GetRawData" method on the same interface, because they'll turn into two GetRawData virtual functions on the C++ side and MSVC will arbitrarily reorder same-name virtual functions in a vtable, so XPConnect won't be able to make sure it's calling the right one when you call from JS.  You need to use different names for the two things.

Comment 31

[Pranav Kant [:pranavk]]

No, I have not applied for auto test requests to the try server. I will write tests and read the instructions as mentioned in the link and apply to the try server.

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> > 3. The function name seems to have a unnecessary "c".
> 
> You can't have both a "rawData" attribute and a "GetRawData" method on the
> same interface, because they'll turn into two GetRawData virtual functions
> on the C++ side and MSVC will arbitrarily reorder same-name virtual
> functions in a vtable, so XPConnect won't be able to make sure it's calling
> the right one when you call from JS.  You need to use different names for
> the two things.
Oh, I didn't thought about that in the first place. How about change the name like GetDataInTArray so we can avoid the name conflict and preserve the meaning of the return type at the same time?

Comment 33

[Pranav Kant [:pranavk]]

I have applied for the commit access ( level 1) here (https://bugzilla.mozilla.org/show_bug.cgi?id=961472). I need someone to vouch for me there.

Comment 34

[Pranav Kant [:pranavk]]

Need vouch here (https://bugzilla.mozilla.org/show_bug.cgi?id=961472)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Going to let schien handle that...

Comment 36

[Pranav Kant [:pranavk]]

Attachment: b952927.patch

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@bz, I don't have level-3 access. Could you help vouch @pranav?

Comment 38

[Pranav Kant [:pranavk]]

Attachment: test_bug952927.patch

Added a xpcshell-test case for rawData.

Comment 39

[Pranav Kant [:pranavk]]

Attachment: cpptest.patch

Extended CPP test (netwerk/test/TestUDPSocket.cpp) to include the CGetRawData() method and verified its content.

Comment 40

[Pranav Kant [:pranavk]]

Attachment: cpptest.patch

Made minor modification to earlier one.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8362247 [details] [diff] [review]
b952927.patch

See comment 29....

Comment 42

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8362927 [details] [diff] [review]
test_bug952927.patch

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

Since this is the first patch, I just want you to know how to write the code as compact/expressive as you can. :)

::: netwerk/test/unit/test_bug952927.js
@@ +1,1 @@
> +var data = new Uint8Array([65,66,67,68]);

You may use "const" here to prevent it being changed accidentally.

@@ +1,2 @@
> +var data = new Uint8Array([65,66,67,68]);
> +var count = data.length;

Using |data.length| directly

@@ +2,5 @@
> +var count = data.length;
> +var str = "127.0.0.1";
> +
> +function checkMessageContent(rawData){
> +    for(var i = 0; i < count; i++){

Please use 2-space indent in the entire file.

@@ +12,5 @@
> +
> +function UDPSocketListener(){
> +}
> +
> +UDPSocketListener.prototype = {

You need to implement |QueryInterface|, i.e. |QueryInterface: XPCOMUtils.generateQI([Ci.nsIUDPSocketListener]),|

@@ +16,5 @@
> +UDPSocketListener.prototype = {
> +    onPacketReceived : function(aSocket, aMessage){
> +	do_print("Packet received on port " + aSocket.port);
> +	if(!checkMessageContent(aMessage.rawData)){
> +	    do_throw("Wrong Data received.");

Please use |do_check_eq| in the entire test case. It's more expressive than |do_throw| here.

@@ +18,5 @@
> +	do_print("Packet received on port " + aSocket.port);
> +	if(!checkMessageContent(aMessage.rawData)){
> +	    do_throw("Wrong Data received.");
> +	}
> +	do_print("Data received matches the data sent.");

You should be able to remove this |do_print| after changing to |do_check_eq|.

@@ +28,5 @@
> +
> +
> +function run_test(){
> +    var cudp = Cc["@mozilla.org/network/udp-socket;1"];
> +    var iudp = cudp.createInstance(Ci.nsIUDPSocket);

You can combined these two lines and give a more expressive variable name.
var socket = Cc["@mozilla.org/network/udp-socket;1"].createInstance(Ci.nsIUDPSocket);

@@ +33,5 @@
> +
> +    iudp.init(-1, true);
> +    do_print("Port assigned is : " + iudp.port);
> +    listener = new UDPSocketListener();
> +    iudp.asyncListen(listener);

Use |socket.asyncListen(new UDPSocketListener());| directly.

@@ +35,5 @@
> +    do_print("Port assigned is : " + iudp.port);
> +    listener = new UDPSocketListener();
> +    iudp.asyncListen(listener);
> +
> +    var written = iudp.send(str, iudp.port, data, count);

use "localhost" or "127.0.0.1" directly, You don't need the |str| variable because it is only used in here.

@@ +37,5 @@
> +    iudp.asyncListen(listener);
> +
> +    var written = iudp.send(str, iudp.port, data, count);
> +    if(written!=count)
> +	do_throw("Data written do not matches the data sent.");

Use |do_check_eq| here.

Comment 43

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8362957 [details] [diff] [review]
cpptest.patch

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

f+ with my comment addressed, forward the review to @mcmanus.

::: netwerk/test/TestUDPSocket.cpp
@@ +59,5 @@
>  
> +  const char* rawBuffer = aMessage->CGetRawData();
> +  uint32_t rawLen = data.Length();
> +
> +  if(len != rawLen){

nit: extra space after "if" and ")". e.g. |if (len != rawLen) {|

@@ +60,5 @@
> +  const char* rawBuffer = aMessage->CGetRawData();
> +  uint32_t rawLen = data.Length();
> +
> +  if(len != rawLen){
> +    fail("Raw data length do not matches String data length.");

You can also print out |rawLen| and expected value in the error message. That will provide basic debug information in the test report.

@@ +63,5 @@
> +  if(len != rawLen){
> +    fail("Raw data length do not matches String data length.");
> +    return false;
> +  }
> +  

nit: remove tailing space.

@@ +65,5 @@
> +    return false;
> +  }
> +  
> +  for (uint32_t i = 0; i < len; i++) {
> +    if(buffer[i] != rawBuffer[i]){

nit: extra space in here, too.

@@ +66,5 @@
> +  }
> +  
> +  for (uint32_t i = 0; i < len; i++) {
> +    if(buffer[i] != rawBuffer[i]){
> +	fail("Raw data do not matches String data.");

Print out value in the error message here, too.

@@ +70,5 @@
> +	fail("Raw data do not matches String data.");
> +	return false;
> +      }
> +  }
> +  

nit: here too.

Comment 44

[Pranav Kant [:pranavk]]

Attachment: b952927

Comment 45

[Pranav Kant [:pranavk]]

Attachment: b952927_xpcshell

Comment 46

[Pranav Kant [:pranavk]]

Attachment: b952927_cppunittest

Comment 47

[Pranav Kant [:pranavk]]

TBPL link : https://tbpl.mozilla.org/?tree=Try&rev=c229c9c0914b

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8363553 [details] [diff] [review]
b952927

Your QI impl doesn't QI to nsIUDPMessage anymore.  Did you test this patch?

>+  mJsobj = mozilla::dom::Uint8Array::Create(cx, 0, mData.Length(), mData.Elements());

Please pass nullptr, not 0.

Also, you only want to do this if the value is not set yet, I'd think.

>+/* [noscript notxpcom nostdcall] nsresult cGetRawData(); */

There's no nsresult involved there.  And this still needs a better name, right?

>-  NS_DECL_THREADSAFE_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

Um, this is a serious problem.  Is this class really used on multiple threads at once?  Cycle-collected things can only be reference counted on a single thread.

>+  JS::Heap<JSObject*> mJsobj;

Could probably use a better name.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8363554 [details] [diff] [review]
b952927_xpcshell

Please fix up the indentation?  Tabs snuck in?

>+		do_print("Packet received on port " + aSocket.port);

Is this really needed?

>+			do_check_eq(aSocket.rawData, data);

checkMessageContent?  Again, was this tested?

Comment 50

[Pranav Kant [:pranavk]]

(In reply to Boris Zbarsky [:bz] from comment #48)

> >-  NS_DECL_THREADSAFE_ISUPPORTS
> >+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> 
> Um, this is a serious problem.  Is this class really used on multiple
> threads at once?  Cycle-collected things can only be reference counted on a
> single thread.
> 

I guess this class is used on multiple threads at once and if Cycle-collected things can only be referenced on a single thread, it is impossible to make this class threadsafe and CC simultaneously. How we are expected to tackle this situation ?

Comment 51

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

nsUDPMessage is created on STS thread and then pass to the other thread that inject the listener, so it needs to be thread-safe. We might need something like a weak reference to a cycle-collected object for the "mJsObj" so that we can keep nsUDPMessage thread safe and recreate the uint8array if the previous one get cycle-collected. However I don't have any example on how we can do it in Gecko now. Maybe @bz can shed some light on this or we just have to ask around.

Comment 52

[Boris Zbarsky [:bzbarsky]]

The other option is that instead of passing an nsUDPMessage object you just pass its data and the object gets constructed on the correct thread...

> We might need something like a weak reference to a cycle-collected object for the "mJsObj"

That's not particularly easy to do while providing a sane JS API, for what it's worth.

Comment 53

[Pranav Kant [:pranavk]]

(In reply to Boris Zbarsky [:bz] from comment #52)
> The other option is that instead of passing an nsUDPMessage object you just
> pass its data and the object gets constructed on the correct thread...

I don't understand how that can still make the method GetRawData() thread-safe. The only way I can see this method to become thread-safe is when you make a local copy of JSObject but in that case there will be leakage.

Comment 54

[Boris Zbarsky [:bzbarsky]]

GetRawData can't be threadsafe while exposing a sane JS API.

Comment 55

[Patrick McManus [:mcmanus]]

Comment on attachment 8363555 [details] [diff] [review]
b952927_cppunittest

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

I'll let you resubmit this when the main patch is updated to a r+'d version

Comment 56

[Patrick McManus [:mcmanus]]

Comment on attachment 8363554 [details] [diff] [review]
b952927_xpcshell

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

I'll let you resubmit this when the main patch is updated to a r+'d version

Comment 57

[Patrick McManus [:mcmanus]]

Comment on attachment 8363553 [details] [diff] [review]
b952927

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

this is all interface changes.. the necko concept is fine by me (f+), and nobody is more qualified than :bz to look at the details. so his r+ is fine by me as long as you're already working together.

Comment 58

[Pranav Kant [:pranavk]]

Attachment: WIP : Message integrity breaks with UDPMessageProxy

The patch though compiles successfully but fails to give accurate results. The cppunittest TestUDPSocket fails. The message created indirectly via UDPMessageProxy does not conform to the actual message expected.

Comment 59

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8372878 [details] [diff] [review]
WIP : Message integrity breaks with UDPMessageProxy

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

::: netwerk/base/public/nsIUDPSocket.idl
@@ +213,5 @@
> +     * Raw Data of the message
> +     */
> +    [implicit_jscontext]
> +    readonly attribute jsval rawData;
> +    [noscript, notxpcom, nostdcall] charPtr cGetRawData();

change the return type to nsTArray<uint8_t>, it'll be more convenient. Also, remember to change the function name in your next upload.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +192,5 @@
>  
>  /* readonly attribute ACString data; */
>  NS_IMETHODIMP nsUDPMessage::GetData(nsACString & aData)
>  {
> +  aData.Assign(reinterpret_cast<const char*>(mData.Elements()), mData.Length());

|static_cast| should be enough.

@@ +339,5 @@
>    mPollFlags = (PR_POLL_READ | PR_POLL_EXCEPT);
>    return NS_OK;
>  }
>  
> +namespace {

merge the anonymous namespace with the original one in line #536. use forward declaration or move the entire block up, if you encounter compile error.

@@ +344,5 @@
> +
> +class UDPMessageProxy MOZ_FINAL : public nsIUDPMessage
> +{
> +public:
> +	UDPMessageProxy(PRNetAddr* aAddr,

nit: fix the indentation.

@@ +349,5 @@
> +									nsIOutputStream* aOutputStream,
> +									nsTArray<uint8_t>& aData)
> +		:mOutputStream(aOutputStream)
> +	{
> +		memcpy(&mAddr, aAddr, sizeof(NetAddr));

should be |sizeof(PRNetAddr)|, although the definition of NetAddr and PRNetAddr are identical. please remember to fix it in nsUDPMessage, too.

@@ +365,5 @@
> +
> +	NS_IMPL_ISUPPORTS1(UDPMessageProxy, nsIUDPMessage)
> +
> +/* readonly attribute nsINetAddr from; */
> +NS_IMETHODIMP UDPMessageProxy::GetFromAddr(nsINetAddr * *aFromAddr)

nit: split return type and function name in two lines. e.g.

NS_IMETHODIMP
UDPMessageProxy::GetFromAddr(nsINetAddr * *aFromAddr)

You can fix the style problem in original code as well.

@@ +381,5 @@
> +
> +/* readonly attribute ACString data; */
> +NS_IMETHODIMP UDPMessageProxy::GetData(nsACString & aData)
> +{
> +  return NS_OK;

return NS_ERROR_NOT_IMPLEMENTED here because you don't want other people to invoke this function.

@@ +712,5 @@
>  NS_IMETHODIMP
>  SocketListenerProxy::OnPacketReceivedRunnable::Run()
>  {
> +	PRNetAddr prAddr;
> +	nsINetAddr* nsAddr;

always use auto pointer to prevent memory leak, i.e. nsCOMPtr<nsINetAddr>.

@@ +721,5 @@
> +	uint32_t count;
> +
> +	mMessage->GetFromAddr(&nsAddr);
> +	nsAddr->GetNetAddr(&addr);
> +	NetAddrToPRNetAddr(&addr, &prAddr);

You can remove the netaddr conversion by changing the address type in nsUDPMessage to NetAddr.

@@ +726,5 @@
> +
> +	mMessage->GetOutputStream(&outputStream);
> +	tData = mMessage->CGetRawData();
> +	count = sizeof(tData);
> +	data.AppendElements(tData, count);

You can remove the array type conversion by changing the return type of CGetRawData().

Comment 60

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

see my feedback in comment #59.

Comment 61

[Pranav Kant [:pranavk]]

And any idea about why the message integrity is not intact by incorporating UDPMessageProxy into the code ?

Comment 62

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Pranav Kant from comment #61)
> And any idea about why the message integrity is not intact by incorporating
> UDPMessageProxy into the code ?

|count = sizeof(tData);| is not a correct way to get the size because the type of tData is char*.

Comment 63

[Pranav Kant [:pranavk]]

Attachment: b952927 (Main patch)

Comment 64

[Pranav Kant [:pranavk]]

Comment on attachment 8373894 [details] [diff] [review]
b952927 (Main patch)

Compiling successfully.

> |static_cast| should be enough.

It breaks the build. So no changes made.

Comment 65

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8373894 [details] [diff] [review]
b952927 (Main patch)

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

It's getting close to a landable patch. Please upload your next patch with my feedback addressed and indentation fixed, with a try result link of your patch.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +11,5 @@
>  interface nsISocketTransport;
>  interface nsIOutputStream;
>  
>  %{ C++
> +#include "nsTArray.h"

|#include "nsTArrayForwardDeclare.h"| should be better.

@@ +20,5 @@
>  }
>  %}
>  native NetAddr(mozilla::net::NetAddr);
>  [ptr] native NetAddrPtr(mozilla::net::NetAddr);
> +[ref] native NsTArray8(nsTArray<uint8_t>);

How about using |Uint8TArrayRef| as the type name?

@@ +213,5 @@
> +
> +		/**
> +		 * The length of the message
> +		 */
> +		readonly attribute unsigned long dataLength;

You don't need the |mDataLength| because you can get this information from |mData.Length()|. Please clean up related code in your next upload.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +214,5 @@
> +NS_IMETHODIMP
> +nsUDPMessage::GetRawData(JSContext* cx,
> +												 JS::MutableHandleValue aRawData)
> +{
> +  mJsobj = mozilla::dom::Uint8Array::Create(cx, 0, mData.Length(), mData.Elements());

Uint8Array will be created every time someone invokes GetRawData. You need to bypass the array creation if mJsobj already hold one.

@@ +224,5 @@
> +/* [noscript, notxpcom, nostdcall] void getDataAsTArray(out NsTArray8 aArray); */
> +void
> +nsUDPMessage::GetDataAsTArray(nsTArray<uint8_t>& aData)
> +{
> +	mData.SwapElements(aData);

This will destroy mData. I would like this function to return |nsTArray<uint8_t>&| directly, with the corresponding change in IDL. UDPMessageProxy should be fixed in the same way.

@@ +526,5 @@
> +	nsCOMPtr<nsIOutputStream> outputStream;
> +	nsTArray<uint8_t> data;
> +	uint32_t count;
> +
> +	mMessage->GetFromAddr(getter_AddRefs(nsAddr));

I would like to see variable declaration being close to where it been used. e.g. http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaPermissionGonk.cpp?from=mediapermissiongonk.cpp#305.

@@ +528,5 @@
> +	uint32_t count;
> +
> +	mMessage->GetFromAddr(getter_AddRefs(nsAddr));
> +	nsAddr->GetNetAddr(&addr);
> +	NetAddrToPRNetAddr(&addr, &prAddr);

Change the constructor of nsUDPMessage to accept NetAddr directly, so that you can get rid of the type conversion between NetAddr and PRNetAddr.

Comment 66

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

See my feedback in comment #65.

p.s. remember to use |feedback?| flag next time.

Comment 67

[Pranav Kant [:pranavk]]

Attachment: Part 1 : Expose raw data on UDP socket messages

https://tbpl.mozilla.org/?tree=Try&rev=7699b647842d

Comment 68

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8374049 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

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

I think your text editor still generate \t, that's why there is still many indentation problems in your patch. I saw the TestUDPSocket is crashed in your try run. Please upload your next patch with all my comment addressed, crash fixed, and set r? flag to @bz.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +214,5 @@
> +    /**
> +     * Raw Data of the message
> +     */
> +    [implicit_jscontext]
> +    readonly attribute jsval rawData;

Merge these two lines into one, so that you can have a unified coding style.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +163,5 @@
> +  mozilla::DropJSObjects(this);
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +nsUDPMessage::nsUDPMessage(NetAddr* aAddr,
> +													 nsIOutputStream* aOutputStream,

nit: fix indent

@@ +164,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +nsUDPMessage::nsUDPMessage(NetAddr* aAddr,
> +													 nsIOutputStream* aOutputStream,
> +													 nsTArray<uint8_t>& aData)

nit: fix indent

@@ +170,2 @@
>  {
> +  memcpy(&mAddr, aAddr, sizeof(PRNetAddr));

no need to change this line because you're passing NetAddr in argument now.

@@ +182,4 @@
>  {
>    NS_ENSURE_ARG_POINTER(aFromAddr);
>  
> +	nsCOMPtr<nsINetAddr> result = new nsNetAddr(&mAddr);

nit: fix indent

@@ +207,5 @@
>  
> +/* readonly attribute jsval rawData; */
> +NS_IMETHODIMP
> +nsUDPMessage::GetRawData(JSContext* cx,
> +												 JS::MutableHandleValue aRawData)

nit: fix indent

@@ +212,5 @@
> +{
> +	if(!mJsobj){
> +		mJsobj = mozilla::dom::Uint8Array::Create(cx, nullptr, mData.Length(), mData.Elements());
> +		mozilla::HoldJSObjects(this);
> +	}

nit: fix indent

@@ +221,5 @@
> +/* [noscript, notxpcom, nostdcall] Uint8ArrayRef getDataAsTArray(); */
> +nsTArray<uint8_t>&
> +nsUDPMessage::GetDataAsTArray()
> +{
> +	return mData;

nit: fix indent

@@ +348,5 @@
> +namespace {
> +//-----------------------------------------------------------------------------
> +// UDPMessageProxy
> +//-----------------------------------------------------------------------------
> +class UDPMessageProxy MOZ_FINAL : public nsIUDPMessage

nit: fix indent in this class definition

@@ +356,5 @@
> +									nsIOutputStream* aOutputStream,
> +									nsTArray<uint8_t>& aData)
> +		:mOutputStream(aOutputStream)
> +	{
> +		memcpy(&mAddr, aAddr, sizeof(PRNetAddr));

|sizeof(NetAddr)| here.

@@ +394,5 @@
> +/* [noscript, notxpcom, nostdcall] Uint8TArrayRef getDataAsTArray(); */
> +nsTArray<uint8_t>&
> +UDPMessageProxy::GetDataAsTArray()
> +{
> +	return mData;

nit: fix indent

@@ +400,5 @@
> +
> +/* readonly attribute jsval rawData; */
> +NS_IMETHODIMP
> +UDPMessageProxy::GetRawData(JSContext* cx,
> +														JS::MutableHandleValue aRawData)

nit: fix indent

@@ +495,5 @@
> +
> +NS_IMETHODIMP
> +SocketListenerProxy::OnPacketReceivedRunnable::Run()
> +{
> +	NetAddr netAddr;

nit: fix indent in this function.

@@ +504,5 @@
> +	nsCOMPtr<nsIOutputStream> outputStream;
> +	mMessage->GetOutputStream(getter_AddRefs(outputStream));
> +
> +	nsTArray<uint8_t> data;
> +	data = mMessage->GetDataAsTArray();

This should be a nsTArray<uint8_t>&, otherwise assignment will still incur memory copy.

@@ +506,5 @@
> +
> +	nsTArray<uint8_t> data;
> +	data = mMessage->GetDataAsTArray();
> +
> +	nsCOMPtr<nsUDPMessage> message = new nsUDPMessage(&netAddr,

Should be |nsCOMPtr<nsIUDPMessage>|.

@@ +649,5 @@
>      return;
>    }
>  
> +	NetAddr netAddr;
> +	PRNetAddrToNetAddr(&prClientAddr, &netAddr);

nit: fix indent

::: netwerk/base/src/nsUDPSocket.h
@@ +67,3 @@
>    NS_DECL_NSIUDPMESSAGE
>  
> +		nsUDPMessage(mozilla::net::NetAddr* aAddr,

nit: fix indent

@@ +72,5 @@
>  
>  private:
>    virtual ~nsUDPMessage();
>  
> +	mozilla::net::NetAddr mAddr;

nit: fix indent

Comment 69

[Pranav Kant [:pranavk]]

Attachment: Part 1 : Expose raw data on UDP socket messages

Will post try link as soon as it starts operating. Closed as of now.

Comment 70

[Pranav Kant [:pranavk]]

https://tbpl.mozilla.org/?tree=Try&rev=d9c171489b4f

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8374897 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

The DropJSObjects should be in the destructor.

Apart from that r=me on the binding/js/cc bits.

Why did we need to move all that code around, though?

Comment 72

[Pranav Kant [:pranavk]]

 > Why did we need to move all that code around, though?

I created a new class UDPMessageProxy in the anonymous namespace. The class SocketListenerProxy was already declared in anonymous namespace. I merged both these classes in one single anonymous namespace which is the reason for moving code around.

Comment 73

[Boris Zbarsky [:bzbarsky]]

It would be better to just leave them in separate anonymous namespaces (or put your new code next to the existing code, instead of moving the existing code) and not confuse the blame, I believe...  Would also make the patch _way_ easier to review.

Comment 74

[Pranav Kant [:pranavk]]

Attachment: Part 2 : cppunittest for Bug 952927

Comment 75

[Pranav Kant [:pranavk]]

Attachment: Part 1 : Expose raw data on UDP socket messages

Comment 76

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@pranav, please update your patch comment and append r=<someone>. For part 1, you should append r=bz,mcmanus, and r=mcmanus for part 2.

Comment 77

[Pranav Kant [:pranavk]]

Attachment: Part 3 : xpcshell-test for Bug 952927

Comment 78

[Josh Matthews [:jdm]]

Please note that this is changing a previously-fallible allocation in OnSocketReady to an infallible one.

Comment 79

[Pranav Kant [:pranavk]]

(In reply to Josh Matthews [:jdm] (on vacation until 2/19) from comment #78)
> Please note that this is changing a previously-fallible allocation in
> OnSocketReady to an infallible one.

Yes. This thing was causing few tests to fail. Converting Infallible to Fallible allocation corrected those.
Thanks.

Comment 80

[Pranav Kant [:pranavk]]

Attachment: Part 1 : Expose raw data on UDP socket messages

This patch is a slight modification of the earlier one : converting infallible allocations to fallible one as corrected by :jdm in Comment 78.

Comment 81

[Pranav Kant [:pranavk]]

Attachment: Part 2 : Extended TestUDPSocket to add raw data functionality.

This bug is a slight modification converting infallible array (nsTArray) to FallibleTArray.

Comment 82

[Pranav Kant [:pranavk]]

Attachment: Part 3 : Added xpcshell-test for this bug.

Comment 83

[Pranav Kant [:pranavk]]

TBPL link incorporating all above mentioned patches :
 
https://tbpl.mozilla.org/?tree=Try&rev=652f29a14e18

All builds and tests successful. Observed frequent intermittent failure on B2G ICS Emulator Opt (mochitest-4) but tests passed after few retries.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376794 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

>+++ b/netwerk/base/src/nsUDPSocket.cpp
> nsUDPMessage::~nsUDPMessage()
> {
>+	mozilla::DropJSObjects(this);

Please fix the indent.  In particular, it looks like you had a tab sneak in there?

>+class UDPMessageProxy MOZ_FINAL : public nsIUDPMessage

I assume this is only an nsIUDPMessage because you want to pass it to OnPacketReceived, since it doesn't actually implement most of the API or anything?

It might be nice to document that, at least; I guess let's now worry about fixing the OnPacketReceived API for now.

>@@ -328,21 +443,21 @@ nsUDPSocket::OnSocketReady(PRFileDesc *f
>+  if(!data.AppendElements(buff, count)){
>+		mCondition = NS_ERROR_UNEXPECTED;
>+		return;
>+	}

More indentation issues.

> SocketListenerProxy::OnPacketReceivedRunnable::Run()
>+  nsCOMPtr<nsIUDPMessage> message = new nsUDPMessage(&netAddr,
>+                                                    outputStream,
>+                                                    data);

And here.

r=me with those fixed.

Comment 85

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376796 [details] [diff] [review]
Part 3 : Added xpcshell-test for this bug.

Looks great, thanks!

Comment 86

[Pranav Kant [:pranavk]]

Attachment: Part 1 : Expose raw data on UDP socket messages

Carry r+ from bz since only indentation fixes are done in this revised patch.

Comment 87

[Pranav Kant [:pranavk]]

Though the Part 1 works fine but it issues following warnings upon running TestUDPSocket.

    [26273] WARNING: NS_ENSURE_TRUE(thread) failed: file /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp, line 123
    [26273] WARNING: unable to post continuation event: file /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp, line 441
    [26273] WARNING: NS_ENSURE_TRUE(thread) failed: file /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp, line 123
    [26273] WARNING: unable to post continuation event: file /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp, line 441
    [26273] WARNING: NS_ENSURE_TRUE(thread) failed: file /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp, line 123
    [26273] WARNING: unable to post continuation event: file /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp, line 441
    [26273] WARNING: NS_ENSURE_TRUE(thread) failed: file /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp, line 123
    [26273] WARNING: unable to post continuation event: file /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp, line 441 

Upon investigating on this issue I took a call stack snapshot using gdb. It is observed that these warnings are generated only after the TestUDPSocket closes the server. 
Following is the call stack before TestUDPSocket closes the server :

    #0  nsSocketTransportService::Dispatch (this=<optimized out>, event=0x655de0, flags=0)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp:123
    #1  0x00007ffff316105a in PostEvent (s=s@entry=0x6558a0, func=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsUDPSocket.cpp:47
    #2  0x00007ffff31633d7 in nsUDPSocket::AsyncListen (this=0x6558a0, aListener=0x656820)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsUDPSocket.cpp:844
    #3  0x0000000000406690 in main (argc=<optimized out>, argv=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/test/TestUDPSocket.cpp:243 

After the server is closed, above mentioned warnings are displayed and following callstack is observed at that moment.

    #0  nsSocketTransportService::Dispatch (this=<optimized out>, event=0x7fffd0007e10, flags=0)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp:123
    #1  0x00007ffff30b8e80 in nsAStreamCopier::PostContinuationEvent_Locked (this=this@entry=0x7fffd0007e00)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp:437
    #2  0x00007ffff30baa91 in PostContinuationEvent_Locked (this=0x7fffd0007e00)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp:433
    #3  nsAStreamCopier::PostContinuationEvent (this=0x7fffd0007e00) at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp:428
    #4  0x00007ffff30baab1 in nsAStreamCopier::OnInputStreamReady (this=<optimized out>, source=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp:392
    #5  0x00007ffff30b1566 in nsPipeEvents::~nsPipeEvents (this=0x7fffffffda10, __in_chrg=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsPipe3.cpp:592
    #6  0x00007ffff30b1bcc in nsPipe::OnPipeException (this=0x7fffd0007b10, reason=reason@entry=NS_BASE_STREAM_CLOSED, outputOnly=<optimized out>,
        outputOnly@entry=true) at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsPipe3.cpp:565
    #7  0x00007ffff30b1dff in nsPipeOutputStream::CloseWithStatus (this=0x7fffd0007b78, reason=NS_BASE_STREAM_CLOSED)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsPipe3.cpp:1064
    #8  0x00007ffff30add1f in nsPipeOutputStream::Release (this=0x7fffd0007b78) at /home/pranavk/pro/mozilla/mozilla-central/xpcom/io/nsPipe3.cpp:1051
    #9  0x00007ffff31642ec in nsUDPMessage::~nsUDPMessage (this=0x65b060, __in_chrg=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/base/src/nsUDPSocket.cpp:178
    #10 0x00007ffff308eefd in SnowWhiteKiller::~SnowWhiteKiller (this=0x7fffffffdb38, __in_chrg=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/base/nsCycleCollector.cpp:2328
    #11 0x00007ffff3087836 in nsCycleCollector::FreeSnowWhite (this=this@entry=0x482620, aUntilNoSWInPurpleBuffer=aUntilNoSWInPurpleBuffer@entry=true)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/base/nsCycleCollector.cpp:2483
    #12 0x00007ffff3087f51 in nsCycleCollector::Shutdown (this=0x482620)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/base/nsCycleCollector.cpp:3373
    #13 0x00007ffff3087fd5 in nsCycleCollector_shutdown () at /home/pranavk/pro/mozilla/mozilla-central/xpcom/base/nsCycleCollector.cpp:3786
    #14 0x00007ffff306cc98 in mozilla::ShutdownXPCOM (servMgr=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/xpcom/build/nsXPComInit.cpp:804
    #15 0x000000000040603d in ScopedXPCOM::~ScopedXPCOM (this=0x7fffffffdd40, __in_chrg=<optimized out>) at ../../dist/include/testing/TestHarness.h:143
    #16 0x00000000004069e3 in main (argc=<optimized out>, argv=<optimized out>)
        at /home/pranavk/pro/mozilla/mozilla-central/netwerk/test/TestUDPSocket.cpp:286 


After discussion with few people on IRC, it seems that such warnings are normal as seen all around build. Moreover, since the mainthread closes the server, I expect these warnings to be normal.

Thus as a confusion, I am not able to decide if these warnings are normal or need to be fixed.

Comment 88

[Boris Zbarsky [:bzbarsky]]

I don't know anything about TestUDPSocket, sorry.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know anything about it either. Try Jason Duell.

Comment 90

[Jason Duell]

Patrick's already needinfo'd, and he's a good person to ask.

Comment 91

[Patrick McManus [:mcmanus]]

(In reply to Pranav Kant [:pranavk] from comment #87)
> Though the Part 1 works fine but it issues following warnings upon running
> TestUDPSocket.
> 
[..]
> 
> 
> After discussion with few people on IRC, it seems that such warnings are
> normal as seen all around build. Moreover, since the mainthread closes the
> server, I expect these warnings to be normal.
> 
> Thus as a confusion, I am not able to decide if these warnings are normal or
> need to be fixed.

I apologize for the delay - I made a mistake in reading the above comment and misinterpreted it to be reporting something much more severe and complicated than it really deals with.

upon further review - The situation is business as usual during shutdown - it probably should not be warning. 

again I apologize for the delay - my mistake.

Comment 92

[Pranav Kant [:pranavk]]

Comment on attachment 8378443 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

carry r+

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14313e0ce5fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74bf7d090ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e1893cb0f9

Comment 94

[Ed Morley [:emorley]]

Backed out for xpcshell failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35792391&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1293344f5eb6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7656a2a456
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a31e6b616085

Comment 95

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi Pranav, please provide a full try run after you fix the xpcshell failure.

Comment 96

[Pranav Kant [:pranavk]]

https://tbpl.mozilla.org/?tree=Try&rev=56b3a275903f

Comment 97

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@pranav, looks like the TPBL is broken before executing rooting hazard analysis. You might want to push a clean try to make sure it is a test machine problem.

Comment 98

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I push the patches with latest m-c on try again. It looks pretty stable now. However, I don't see any patch update after the backout. @pranav, do you know what goes wrong for your previous push?

https://tbpl.mozilla.org/?tree=Try&rev=782fd5331f81

Comment 99

[Pranav Kant [:pranavk]]

Nah, I don't have any idea why my previous push with same patches went wrong. It never gave such stable results ever before.

Comment 100

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Well, let's try land this patch again.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

I see IDL changes without a UUID bump? That means the build system won't necessarily know to rebuild certain files and would likely lead to the results you saw here. Try builds are always clobbers, explaining the green there.

Comment 102

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Nice catch, @Ryan!

@pranav, please update the UUID for nsIUDPSocket.idl. You can generate one via |./mach uuid|.

Comment 103

[Pranav Kant [:pranavk]]

I am afraid but that still doesn't solve the problem when I pushed to try.
https://tbpl.mozilla.org/?tree=Try&rev=272fb39ded80

Comment 104

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I just take a look at your m-c base version. You're still using the code base from Feb. 14th. My last try is based on Mar. 12th and it's all green. This red flag on Hf should be solved already and you just need to rebase your patch to latest m-c.

Comment 105

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - Expose raw data on UDP socket messages

rebase to m-c tip and update UUID according to comment, carry r+.

Comment 106

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 : Extended TestUDPSocket to add raw data functionality.

rebase to m-c tip, carry r+.

Comment 107

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 : Added xpcshell-test for this bug.

rebase to m-c tip, carry r+.

Comment 108

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

let's see if everything is still good after rebase.

https://tbpl.mozilla.org/?tree=Try&rev=ad72cd0a5ccd

Comment 109

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/583528a07f5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc218fe81e0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a084084260

Comment 110

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/583528a07f5a
https://hg.mozilla.org/mozilla-central/rev/bc218fe81e0d
https://hg.mozilla.org/mozilla-central/rev/e4a084084260