Eric Rahm [:erahm] Reporter
	Description • 9 years ago

Note: this is only in test code, set priority as you see fit.

Coverity indicates that |expected| in |testXDR_sourceMap| [1] is leaked.

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testXDR.cpp#l117

	shubham_sinha Assignee
	Comment 1 • 9 years ago

hey
i would like to work on this bug

	Eric Rahm [:erahm] Reporter
	Comment 2 • 9 years ago

Hi shubham_sinha, good to see you are still interested in contributing! The solution for this bug is similar to bug 1142826. Please feel free to start working on the bug and let me know if you have any questions.

	Jinank Jain[:jj]
	Comment 3 • 9 years ago

How do you pointer++ in case of Unique Ptr

	Eric Rahm [:erahm] Reporter
	Comment 4 • 9 years ago

(In reply to Jinank Jain from comment #3)
> How do you pointer++ in case of Unique Ptr

Good question Jinank Jain!

In this case it would probably be easiest to do something along the lines of:

> UniquePtr<char16_t *> expected_wrapper = js::InflateString(cx, *sm, &len);
> char16_t *expected = expected_wrapper.get();

But let's give shubham_sinha a chance to work on this as they've shown some interest :)

	shubham_sinha Assignee
	Comment 5 • 9 years ago

hey eric,
how to know if the patch i have created works correctly or not

	Eric Rahm [:erahm] Reporter
	Comment 6 • 9 years ago

(In reply to shubham_sinha from comment #5)
> hey eric,
> how to know if the patch i have created works correctly or not

You can find some pretty good details about the process at: https://developer.mozilla.org/docs/Introduction

First you'll want to test that the patch builds [1], if you've already gone through and set things up this should simply be:
> ./mach build

Next you'll want to run the test to make sure it still passes. To run this specific test you'll need to do a build (you probably already did that) and then run:
> $(OBJ_DIR)/dist/bin/jsapi-tests testMutedErrors

where $(OBJ_DIR) is the where your build output goes. For me this is |obj-x86_64-unknown-linux-gnu|, it will almost always start with 'obj-'. Example of running with success:
> [erahm@mozilla21268 mozilla-central]$ obj-x86_64-unknown-linux-gnu/dist/bin/jsapi-tests testMutedErrors
> testMutedErrors
> TEST-PASS | testMutedErrors | ok
> 
> Passed: ran 1 tests.

	shubham_sinha Assignee
	Comment 7 • 9 years ago

is it correct?plz review it for me

	Eric Rahm [:erahm] Reporter
	Comment 8 • 9 years ago

Comment on attachment 8606592 [details] [diff] [review]
bug.patch

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

This looks great, we just need to modify it a little bit.

::: js/src/jsapi-tests/testXDR.cpp
@@ +10,5 @@
>  
>  #include "jsapi-tests/tests.h"
>  
>  #include "jsscriptinlines.h"
> +using mozilla::UniquePtr;	

nit: Please remove the trailing spaces.

@@ +128,5 @@
>          CHECK(JS_CompileScript(cx, "", 0, options, &script));
>          CHECK(script);
>  
>          size_t len = strlen(*sm);
> +        UniquePtr<char16_t> expected_wrapper(js::InflateString(cx, *sm, &len));

Looking closer at |js::InflateString| [1] it looks like we'll need to add an extra parameter to |UniquePtr| explaining how to cleanup the memory. Luckily this shouldn't be too bad.

We can use the |JS::FreePolicy| [2] to specify the deleter. A simple change should suffice:
   
>    UniquePtr<char16_t, JS::FreePolicy> expected_wrapper...

All this does is tells |UniquePtr| to use |free| instead of |delete|.

[1] https://hg.mozilla.org/mozilla-central/annotate/0273e9c967ec/js/src/jsstr.cpp#l4573
[2] https://hg.mozilla.org/mozilla-central/annotate/0273e9c967ec/js/public/Utility.h#l333

	shubham_sinha Assignee
	Comment 9 • 9 years ago

Did the changes

	Eric Rahm [:erahm] Reporter
	Comment 10 • 9 years ago

Comment on attachment 8606701 [details] [diff] [review]
bug.patch

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

This looks good! I have a few very minor style comments. Also can you recreate your patch? This doesn't seem to be based on what's in mozilla-central (you might have multiple patches in your queue locally).

When you update the patch, please update it with a proper title and add an r=erahm to the end. There are good details on creating patches here: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: js/src/jsapi-tests/testXDR.cpp
@@ -6,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "jsscript.h"
>  #include "jsstr.h"
> -

This empty line doesn't need to be removed.

@@ -11,2 @@
>  #include "jsapi-tests/tests.h"
> -

This empty line doesn't need to be removed.

@@ +11,2 @@
>  #include "jsscriptinlines.h"
> +using mozilla::UniquePtr;	

nit: Please remove the tab at the end of this line and add an empty line above and below it like so:

> #include "jsscriptinlines.h"
> 
> using mozilla::UniquePtr;
> 
> static JSScript*

	Eric Rahm [:erahm] Reporter
	Comment 12 • 9 years ago

Comment on attachment 8607329 [details] [diff] [review]
bug-fix.patch

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

Looks good, thank you for your contribution! I'll get this landed shortly.

	Eric Rahm [:erahm] Reporter
	Comment 13 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a74c8ef5bf0

	shubham_sinha Assignee
	Comment 14 • 9 years ago

what to do after this regarding this bug?

	Eric Rahm [:erahm] Reporter
	Comment 15 • 9 years ago

(In reply to shubham_sinha from comment #14)
> what to do after this regarding this bug?

We were having some infrastructure problems yesterday, I'll get this landed today.

	Eric Rahm [:erahm] Reporter
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce80e70b04e2

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/ce80e70b04e2