1007136 - Invalid Address specified to RtlValidateHeap from JSAutoByteString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[qiweibo1984]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

When I use spindermonkey dynamic libraries in the windows, spindermonkey compiler does not use debug mode (. / Configure), 
but also in other dynamic library (using the / MDd compilation mode) using this dynamic library in debug mode.


Actual results:

then will produce "Invalid Address specified to RtlValidateHeap" error.


Expected results:

no error.The reason is JSAutoByteString class used JS_EncodeString application memory, but the memory is released using free.

Comment 1

[Nathan Froyd [:froydnj]]

Can you provide the arguments you are passing to configure and a small testcase?

Comment 2

[qiweibo1984]

Attachment: jstest.rar

This attachment is the small case to reproduce the error.
I am using VS2010, Debug mode .

Comment 3

[qiweibo1984]

(In reply to Nathan Froyd (:froydnj) from comment #1)
> Can you provide the arguments you are passing to configure and a small
> testcase?

thinks Nathan Froyd
I'm sorroy for my English, I am Chinese.
yes, I'm using SpiderMonkey is js185-1.0.0.tar.gz, compiler environment is start-msvc10.bat,
Compile command is:
cd js / src
autoconf2.13

mkdir build-release
cd build-release
.. / configure
make 

testcase in the attachment.

Comment 4

[Till Schneidereit [:till]]

(In reply to qiweibo1984 from comment #3)
> yes, I'm using SpiderMonkey is js185-1.0.0.tar.gz

That version is several years old and thus probably not what you want. Check out this page for a link to the current release and additional documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey

Comment 5

[qiweibo1984]

Attachment: using the latest version

well, now I'm using the latest version, but the error still exists.

Comment 6

[qiweibo1984]

(In reply to Till Schneidereit [:till] from comment #4)
> (In reply to qiweibo1984 from comment #3)
> > yes, I'm using SpiderMonkey is js185-1.0.0.tar.gz
> 
> That version is several years old and thus probably not what you want. Check
> out this page for a link to the current release and additional documentation:
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey

using the latest version，project  in the attachment 8419461 [details]
well, now I'm using the latest version, but the error still exists.

Comment 7

[Mihai Dobrescu]

I have come here for the same issue on mozjs-38.3.0sfink2.
I confirm the issue using Visual Studio 2013 Pro.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Isn't the basic problem here that JSAutoByteString is just broken in terms of its malloc/free handling?

It allocates memory via out-of-line methods like JS_EncodeStringToUTF8 and JS_EncodeString, so uses the spidermonkey DLL heap for that.

It frees memory in an inline destructor using the inline js_free method, which uses the consumer DLL's heap.  That's not right.

Seems to me like JSAutoByteString should use JS_free (which is not inlined and hence will use the SpiderMonkey DLL's heap) to free memory, no?  Terrence, sorry to needinfo you but I'm told you're in that part of the Venn diagram where SpiderMonkey hackers who know something about Windows live...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Reporter, if you do change JSAutoByteString::Clear to call JS_free instead of js_free, does that make the problem go away?

Comment 10

[Mihai Dobrescu]

How do you propose to call JS_free? It needs context.
Also, calling directly free will raise the same error.

Comment 11

[Mihai Dobrescu]

I see in jsapi.cpp:

JS_PUBLIC_API(void)
JS_free(JSContext* cx, void* p)
{
    return js_free(p);
}

Comment 12

[Boris Zbarsky [:bzbarsky]]

> How do you propose to call JS_free? It needs context.

Oh, right, that silliness.  Pass null for the JSContext*; it doesn't use that argument, as you discovered.

> Also, calling directly free will raise the same error.

That's all js_free does, sure.  The key part, if I'm right, is _which_ DLL's free is called, and that depends on which DLL the call is compiled into...  That's why I'm suggesting trying the out-of-line JS_free instead of the inline js_free: it causes free to be called from a different DLL.

Comment 13

[Mihai Dobrescu]

Using JS_free(NULL, bytes) works fine.

Comment 14

[Terrence Cole [:terrence]]

Attachment: fix_autobytestring-v0.diff

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712400 [details] [diff] [review]
fix_autobytestring-v0.diff

r=me; was about to write this exact patch.  ;)

Comment 16

[Mihai Dobrescu]

Well, I wouldn't be happy so soon, there are more js_free calls in some points of the code of other classes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I filed bug 1243367 on the remaining issues.

Comment 18

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb479c864986d2a48b1090a5f2409989b96de20
Bug 1007136 - Ensure malloc/free always match when using JSAutoByteString; r=bz

Comment 19

[qiweibo1984]

Thanks All

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eb479c86498

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eb479c86498

Comment 22

[Mihai Dobrescu]

The issue returned in SpiderMonkey 45, taken from https://people.mozilla.org/~sfink/mozjs-45.0.2.tar.bz2 (refered by https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases/45).

Comment 23

[Boris Zbarsky [:bzbarsky]]

This bug is fixed in Firefox 47.

Comment 24

[Boris Zbarsky [:bzbarsky]]

But perhaps we should backport to 45 esr....

Comment 25

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #24)
> But perhaps we should backport to 45 esr....

Seems like a good idea to me!

Comment 26

[Terrence Cole [:terrence]]

Comment on attachment 8712400 [details] [diff] [review]
fix_autobytestring-v0.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: SpiderMonkey bases its releases off of ESR. Without this patch, SpiderMonkey is basically unusable for embedding on windows.
User impact if declined: Substantially more frustration for embedders as they will have to patch the source they get from us and substantially more frustration for us having to help with this until the next ESR hits.
Fix Landed on Version: 47
Risk to taking this patch (and alternatives if risky): Non-existent. The change is trivial; it has been baking in automation for months; and we have a direct report by the first affected embedder that the path fixes their problem.  
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8712400 [details] [diff] [review]
fix_autobytestring-v0.diff

Fix to land in parallel with 47 for esr-45.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/5cb01593c52b