Expose raw data on UDP socket messages

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mikedeboer, Assigned: pranavk)

Tracking

({dev-doc-needed})

Trunk
mozilla31
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=schien][mentor-lang=zh], URL)

Attachments

(3 attachments, 21 obsolete attachments)

12.80 KB, patch
schien
: review+
Details | Diff | Splinter Review
1.38 KB, patch
schien
: review+
Details | Diff | Splinter Review
2.04 KB, patch
schien
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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!
Whiteboard: [good-first-bug][mentor=schien][mentor-lang=zh]

Comment 1

5 years ago
I wouldl like to work on this bug. How can I begin?
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.
> ([array] uint8_t rawData)

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

Comment 5

5 years ago
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.
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.
Are you designing this API to be used from C++ or from JS?

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

Comment 8

5 years ago
> 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.
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.
(Reporter)

Comment 10

5 years ago
(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.
> 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...
(Reporter)

Comment 12

5 years ago
> 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.
(Assignee)

Comment 13

5 years ago
Created attachment 8359703 [details] [diff] [review]
Bug_952927.patch

Require review to my first ever patch to Mozilla. Pardon for any mistakes. Critics are strongly invited to cure me.
Attachment #8359703 - Flags: review?(schien)
(Assignee)

Comment 14

5 years ago
(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 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...
> 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 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.
Attachment #8359703 - Flags: review?(schien)
(Assignee)

Comment 18

5 years ago
Created attachment 8360601 [details] [diff] [review]
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).
Attachment #8359703 - Attachment is obsolete: true
Attachment #8360601 - Flags: review?(schien)
(Assignee)

Comment 19

5 years ago
And can you please assign this this bug against my name.
Assignee: nobody → pranav913
Pranav, have you tested this patch?
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
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 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.
Attachment #8360601 - Flags: review?(schien)
(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.
(Assignee)

Comment 25

5 years ago
Created attachment 8362050 [details] [diff] [review]
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.
Attachment #8360601 - Attachment is obsolete: true
Attachment #8362050 - Flags: review?(schien)
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
Attachment #8362050 - Flags: review?(schien)
Attachment #8362050 - Flags: review?(mcmanus)
Attachment #8362050 - Flags: review?(bzbarsky)
BTW, Start bringing @mcmanus to the review process since he is the necko peer.
@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.
Flags: needinfo?(pranav913)
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]);
  }

?
> 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.
(Assignee)

Comment 31

5 years ago
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.
Flags: needinfo?(pranav913)
(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?
(Assignee)

Comment 33

5 years ago
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.
Flags: needinfo?(schien)
(Assignee)

Comment 34

5 years ago
Need vouch here (https://bugzilla.mozilla.org/show_bug.cgi?id=961472)
Flags: needinfo?(bzbarsky)
Going to let schien handle that...
(Assignee)

Comment 36

5 years ago
Created attachment 8362247 [details] [diff] [review]
b952927.patch
Attachment #8362050 - Attachment is obsolete: true
Attachment #8362050 - Flags: review?(mcmanus)
Attachment #8362050 - Flags: review?(bzbarsky)
Attachment #8362247 - Flags: review?(mcmanus)
Attachment #8362247 - Flags: review?(bzbarsky)
@bz, I don't have level-3 access. Could you help vouch @pranav?
Flags: needinfo?(schien)
(Assignee)

Comment 38

5 years ago
Created attachment 8362927 [details] [diff] [review]
test_bug952927.patch

Added a xpcshell-test case for rawData.
Attachment #8362927 - Flags: review?(schien)
(Assignee)

Comment 39

5 years ago
Created attachment 8362947 [details] [diff] [review]
cpptest.patch

Extended CPP test (netwerk/test/TestUDPSocket.cpp) to include the CGetRawData() method and verified its content.
Attachment #8362947 - Flags: review?(schien)
(Assignee)

Comment 40

5 years ago
Created attachment 8362957 [details] [diff] [review]
cpptest.patch

Made minor modification to earlier one.
Attachment #8362947 - Attachment is obsolete: true
Attachment #8362947 - Flags: review?(schien)
(Assignee)

Updated

5 years ago
Attachment #8362957 - Flags: review?(schien)
Comment on attachment 8362247 [details] [diff] [review]
b952927.patch

See comment 29....
Attachment #8362247 - Flags: review?(bzbarsky) → review-
Flags: needinfo?(bzbarsky)
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.
Attachment #8362927 - Flags: review?(schien)
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.
Attachment #8362957 - Flags: review?(schien)
Attachment #8362957 - Flags: review?(mcmanus)
Attachment #8362957 - Flags: feedback+
(Assignee)

Comment 44

5 years ago
Created attachment 8363553 [details] [diff] [review]
b952927
Attachment #8362247 - Attachment is obsolete: true
Attachment #8362247 - Flags: review?(mcmanus)
Attachment #8363553 - Flags: review?(mcmanus)
Attachment #8363553 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

5 years ago
Created attachment 8363554 [details] [diff] [review]
b952927_xpcshell
Attachment #8362927 - Attachment is obsolete: true
Attachment #8363554 - Flags: review?(mcmanus)
Attachment #8363554 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 46

5 years ago
Created attachment 8363555 [details] [diff] [review]
b952927_cppunittest
Attachment #8362957 - Attachment is obsolete: true
Attachment #8362957 - Flags: review?(mcmanus)
Attachment #8363555 - Flags: review?(mcmanus)
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.
Attachment #8363553 - Flags: review?(bzbarsky) → review-
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?
Attachment #8363554 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 50

5 years ago
(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 ?
Flags: needinfo?(schien)
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.
Flags: needinfo?(schien) → needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 53

5 years ago
(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.
Flags: needinfo?(bzbarsky)
GetRawData can't be threadsafe while exposing a sane JS API.
Flags: needinfo?(bzbarsky)
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
Attachment #8363555 - Flags: review?(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
Attachment #8363554 - Flags: review?(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.
Attachment #8363553 - Flags: review?(mcmanus) → feedback+
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 58

5 years ago
Created attachment 8372878 [details] [diff] [review]
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.
Attachment #8363553 - Attachment is obsolete: true
Flags: needinfo?(schien)
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().
see my feedback in comment #59.
Flags: needinfo?(schien)
(Assignee)

Comment 61

5 years ago
And any idea about why the message integrity is not intact by incorporating UDPMessageProxy into the code ?
(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*.
(Assignee)

Comment 63

5 years ago
Created attachment 8373894 [details] [diff] [review]
b952927 (Main patch)
Attachment #8372878 - Attachment is obsolete: true
Flags: needinfo?(schien)
(Assignee)

Comment 64

5 years ago
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.
Attachment #8373894 - Attachment description: Successful compile. → b952927 (Main patch)
Flags: needinfo?(schien)
(Assignee)

Updated

5 years ago
Flags: needinfo?(schien)
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.
Attachment #8373894 - Flags: feedback-
See my feedback in comment #65.

p.s. remember to use |feedback?| flag next time.
Flags: needinfo?(schien)
(Assignee)

Comment 67

5 years ago
Created attachment 8374049 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

https://tbpl.mozilla.org/?tree=Try&rev=7699b647842d
Attachment #8373894 - Attachment is obsolete: true
Attachment #8374049 - Flags: feedback?(schien)
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
Attachment #8374049 - Flags: feedback?(schien) → feedback+
(Assignee)

Comment 69

5 years ago
Created attachment 8374897 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

Will post try link as soon as it starts operating. Closed as of now.
Attachment #8374049 - Attachment is obsolete: true
Attachment #8374897 - Flags: review?(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?
Attachment #8374897 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 72

5 years ago
 > 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.
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.
(Assignee)

Comment 74

5 years ago
Created attachment 8376464 [details] [diff] [review]
Part 2 : cppunittest for Bug 952927
Attachment #8363555 - Attachment is obsolete: true
Attachment #8376464 - Flags: review?(mcmanus)
(Assignee)

Comment 75

5 years ago
Created attachment 8376465 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages
Attachment #8374897 - Attachment is obsolete: true
Attachment #8376465 - Flags: review?(bzbarsky)
Attachment #8376464 - Flags: review?(mcmanus) → review+
@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.
(Assignee)

Comment 77

5 years ago
Created attachment 8376706 [details] [diff] [review]
Part 3 : xpcshell-test for Bug 952927
Attachment #8363554 - Attachment is obsolete: true
Attachment #8376706 - Flags: review?(mcmanus)
Attachment #8376706 - Flags: feedback?(bzbarsky)
Please note that this is changing a previously-fallible allocation in OnSocketReady to an infallible one.
(Assignee)

Comment 79

5 years ago
(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.
(Assignee)

Comment 80

5 years ago
Created attachment 8376794 [details] [diff] [review]
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.
Attachment #8376465 - Attachment is obsolete: true
Attachment #8376465 - Flags: review?(bzbarsky)
Attachment #8376794 - Flags: review?(bzbarsky)
Attachment #8376794 - Flags: approval-mozilla-release?
(Assignee)

Updated

5 years ago
Attachment #8376794 - Flags: approval-mozilla-release?
(Assignee)

Comment 81

5 years ago
Created attachment 8376795 [details] [diff] [review]
Part 2 :  Extended TestUDPSocket to add raw data functionality.

This bug is a slight modification converting infallible array (nsTArray) to FallibleTArray.
Attachment #8376464 - Attachment is obsolete: true
Attachment #8376795 - Flags: review?(mcmanus)
(Assignee)

Comment 82

5 years ago
Created attachment 8376796 [details] [diff] [review]
Part 3 : Added xpcshell-test for this bug.
Attachment #8376706 - Attachment is obsolete: true
Attachment #8376706 - Flags: review?(mcmanus)
Attachment #8376706 - Flags: feedback?(bzbarsky)
Attachment #8376796 - Flags: review?(mcmanus)
Attachment #8376796 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 83

5 years ago
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.
Attachment #8376795 - Flags: review?(mcmanus) → review+
Attachment #8376796 - Flags: review?(mcmanus) → review+
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.
Attachment #8376794 - Flags: review?(bzbarsky) → review+
Comment on attachment 8376796 [details] [diff] [review]
Part 3 : Added xpcshell-test for this bug.

Looks great, thanks!
Attachment #8376796 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 86

5 years ago
Created attachment 8378443 [details] [diff] [review]
Part 1 : Expose raw data on UDP socket messages

Carry r+ from bz since only indentation fixes are done in this revised patch.
Attachment #8376794 - Attachment is obsolete: true
(Assignee)

Comment 87

5 years ago
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.
Flags: needinfo?(mcmanus)
(Assignee)

Updated

5 years ago
Flags: needinfo?(bzbarsky)
I don't know anything about TestUDPSocket, sorry.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

5 years ago
Flags: needinfo?(roc)
I don't know anything about it either. Try Jason Duell.
Flags: needinfo?(roc) → needinfo?(jduell.mcbugs)

Comment 90

5 years ago
Patrick's already needinfo'd, and he's a good person to ask.
Flags: needinfo?(jduell.mcbugs)
(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.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 92

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

carry r+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Hi Pranav, please provide a full try run after you fix the xpcshell failure.
Flags: needinfo?(pranav913)
(Assignee)

Comment 96

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=56b3a275903f
Flags: needinfo?(pranav913)
@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.

Updated

5 years ago
Whiteboard: [good-first-bug][mentor=schien][mentor-lang=zh] → [good first bug][mentor=schien][mentor-lang=zh]
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
Flags: needinfo?(pranav913)
(Assignee)

Comment 99

5 years ago
Nah, I don't have any idea why my previous push with same patches went wrong. It never gave such stable results ever before.
Flags: needinfo?(pranav913)
Well, let's try land this patch again.
Keywords: checkin-needed
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.
Keywords: checkin-needed
Nice catch, @Ryan!

@pranav, please update the UUID for nsIUDPSocket.idl. You can generate one via |./mach uuid|.
Flags: needinfo?(pranav913)
(Assignee)

Comment 103

5 years ago
I am afraid but that still doesn't solve the problem when I pushed to try.
https://tbpl.mozilla.org/?tree=Try&rev=272fb39ded80
Flags: needinfo?(pranav913)
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.
Created attachment 8396900 [details] [diff] [review]
Part 1 - Expose raw data on UDP socket messages

rebase to m-c tip and update UUID according to comment, carry r+.
Attachment #8378443 - Attachment is obsolete: true
Attachment #8396900 - Flags: review+
Created attachment 8396901 [details] [diff] [review]
Part 2 : Extended TestUDPSocket to add raw data functionality.

rebase to m-c tip, carry r+.
Attachment #8376795 - Attachment is obsolete: true
Attachment #8396901 - Flags: review+
Attachment #8396900 - Attachment description: Expose raw data on UDP socket messages → Part 1 - Expose raw data on UDP socket messages
Created attachment 8396904 [details] [diff] [review]
Part 3 : Added xpcshell-test for this bug.

rebase to m-c tip, carry r+.
Attachment #8376796 - Attachment is obsolete: true
Attachment #8396904 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/583528a07f5a
https://hg.mozilla.org/mozilla-central/rev/bc218fe81e0d
https://hg.mozilla.org/mozilla-central/rev/e4a084084260
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.