Closed Bug 1007136 Opened 5 years ago Closed 4 years ago

Invalid Address specified to RtlValidateHeap from JSAutoByteString

Categories

(Core :: JavaScript Engine, defect, major)

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox-esr45 --- fixed

People

(Reporter: qiweibo1984, Unassigned)

Details

(Keywords: assertion, crash, memory-leak)

Attachments

(3 files)

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.
Keywords: assertion, mlk
Priority: -- → P1
Can you provide the arguments you are passing to configure and a small testcase?
Flags: needinfo?(qiweibo1984)
Attached file jstest.rar
This attachment is the small case to reproduce the error.
I am using VS2010, Debug mode .
Flags: needinfo?(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.
(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
well, now I'm using the latest version, but the error still exists.
(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.
Keywords: crash
Summary: RtlValidateHeap → Invalid Address specified to RtlValidateHeap from JSAutoByteString
Severity: normal → major
Component: JavaScript Engine → JavaScript: Internationalization API
Priority: P1 → P4
Version: unspecified → Trunk
I have come here for the same issue on mozjs-38.3.0sfink2.
I confirm the issue using Visual Studio 2013 Pro.
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...
Status: UNCONFIRMED → NEW
Component: JavaScript: Internationalization API → JavaScript Engine
Ever confirmed: true
Flags: needinfo?(terrence)
Reporter, if you do change JSAutoByteString::Clear to call JS_free instead of js_free, does that make the problem go away?
Flags: needinfo?(qiweibo1984)
How do you propose to call JS_free? It needs context.
Also, calling directly free will raise the same error.
I see in jsapi.cpp:

JS_PUBLIC_API(void)
JS_free(JSContext* cx, void* p)
{
    return js_free(p);
}
> 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.
Using JS_free(NULL, bytes) works fine.
Priority: P4 → --
Flags: needinfo?(terrence)
Attachment #8712400 - Flags: review?(bzbarsky)
Comment on attachment 8712400 [details] [diff] [review]
fix_autobytestring-v0.diff

r=me; was about to write this exact patch.  ;)
Attachment #8712400 - Flags: review?(bzbarsky) → review+
Well, I wouldn't be happy so soon, there are more js_free calls in some points of the code of other classes.
I filed bug 1243367 on the remaining issues.
Flags: needinfo?(qiweibo1984)
Thanks All
https://hg.mozilla.org/mozilla-central/rev/9eb479c86498
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This bug is fixed in Firefox 47.
But perhaps we should backport to 45 esr....
Flags: needinfo?(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!
Flags: needinfo?(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.
Attachment #8712400 - Flags: approval-mozilla-esr45?
Comment on attachment 8712400 [details] [diff] [review]
fix_autobytestring-v0.diff

Fix to land in parallel with 47 for esr-45.
Attachment #8712400 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.