Last Comment Bug 670958 - Javascript 1.8.5 js_GetScriptLineExtent does not always return the correct value
: Javascript 1.8.5 js_GetScriptLineExtent does not always return the correct value
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Other
: -- normal (vote)
: mozilla8
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-12 09:54 PDT by Robin Ehrlich
Modified: 2011-07-13 09:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
getScriptLineExtent.cpp (915 bytes, text/plain)
2011-07-12 09:54 PDT, Robin Ehrlich
no flags Details
diff -u 8 of changes (1.25 KB, patch)
2011-07-12 12:03 PDT, Robin Ehrlich
sphink: review+
Details | Diff | Splinter Review

Description Robin Ehrlich 2011-07-12 09:54:34 PDT
Created attachment 545395 [details]
getScriptLineExtent.cpp

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB6; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; OfficeLiveConnector.1.3; OfficeLivePatch.0.0; .NET4.0C)

Steps to reproduce:

I ran the following script using my application that embeds Javascript:

xx = 1;

try {
	 debugger;

	 xx += 1;
}
catch (e)
{
	 xx += 1;
}        




Actual results:

When the debug hook is called, my application calls JS_GetScriptLineExtent and a small value (2?) is returned. I would expect 12. The reason is that the line numbers as processed by js_GetScriptLineExtent in jsscript.cpp are not sequential.

I have attached a version of js_GetScriptLineExtent that corrects the problem.
Comment 1 David Mandelin [:dmandelin] 2011-07-12 11:11:22 PDT
Could you post that as a patch?
Comment 2 Robin Ehrlich 2011-07-12 11:30:40 PDT
> Could you post that as a patch?

I am sorry, but I don't know what you mean by this. The supplied code is a replacement for the function js_GetScriptLineExtent in jsscript.cpp. The changes are quite short and simple.

Here is a diff:

1924a1925,1926
>
>     bool counting;
1925a1928
>     uintN maxLineNo;
1929a1933,1934
>     maxLineNo = 0;
>     counting = true;
1932a1938,1939
>             if (maxLineNo < lineno)
>                 maxLineNo = lineno;
1933a1941,1945
>             counting = true;
>             if (maxLineNo < lineno)
>                 maxLineNo = lineno;
>             else
>                 counting = false;
1935c1947,1948
<             lineno++;
---
>             if (counting)
>                 lineno++;
1937a1951,1954
>
>     if (maxLineNo > lineno)
>         lineno = maxLineNo;
>
Comment 3 David Mandelin [:dmandelin] 2011-07-12 11:56:05 PDT
(In reply to comment #2)
> > Could you post that as a patch?
> 
> I am sorry, but I don't know what you mean by this.

I mean a diff in the 'unified' format with 8 lines of context. That's the standard form we use to review and take changes. I guess you are using a source release rather than hg, so the output of 'diff -u 8' should work.
Comment 4 Robin Ehrlich 2011-07-12 12:03:30 PDT
Created attachment 545439 [details] [diff] [review]
diff -u 8 of changes
Comment 5 Robin Ehrlich 2011-07-12 12:04:05 PDT
I have added an attachment with a diff -u 8 of the changes.
Comment 6 Steve Fink [:sfink] [:s:] 2011-07-12 14:26:22 PDT
Comment on attachment 545439 [details] [diff] [review]
diff -u 8 of changes

Looks good to me, other than an extra newline at the top. It's funny, I was just looking at that code a few days ago and thinking, "Really? That works? We must *really* constrain our bytecode generation in funky ways!"

I actually pushed this to try an hour ago. If it's green, I'll land it along with some other patches.
Comment 7 Steve Fink [:sfink] [:s:] 2011-07-12 17:56:37 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d1746a8b20a9

That's your patch plus I wrapped up your test code into jsapi-test/testScriptInfo.cpp that tests this plus a few other related APIs.

Note You need to log in before you can comment on or make changes to this bug.