936143 - Fix js_GetScriptLineExtent. (r=?)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix js_GetScriptLineExtent. (r=?)

This is crazy, js_GetScriptLineExtent has been incorrect for ages AFAICT, for
sequences of source notes where, for N > M,

setline N
setline M
newline

When processing setline M, it stops counting newlines because M <= N, so if the
script ends on source notes, it can give back a short line extent. Test
forthcoming.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Fix js_GetScriptLineExtent. (r=?)

Now with test.

Comment 3

[Shu-yu Guo [:shu]]

> script ends on source notes, it can give back a short line extent.

I meant to say if the script ends on newline source notes.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 828864 [details] [diff] [review]
Fix js_GetScriptLineExtent. (r=?)

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

::: js/src/jit-test/tests/debug/Script-startLine.js
@@ +44,5 @@
>  test(g.f2);
>  test(g.f2);
> +
> +// Having a last = Error().lineNumber forces a setline srcnote, so test a
> +// function that ends with of newline srcnotes.

"ends with of"???

Comment 5

[Jim Blandy :jimb]

Attachment: Simplify GetScriptLineExtent.

But why not just take out all that hair, while we're here?

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb230c191a88
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13e98eab890

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out as part of https://hg.mozilla.org/integration/mozilla-inbound/rev/67f5d934127c because something in the push caused various test bustage on Linux ASAN's browser-chrome suite: 

https://tbpl.mozilla.org/php/getParsedLog.php?id=30444281&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30444457&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445445&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445739&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445414&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30446297&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30446502&tree=Mozilla-Inbound

Shu and RyanVM both suspect that https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4011f33422 was the cause for the failures, but I backed out the whole push just to be sure.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9ad4cc32e5d
https://hg.mozilla.org/mozilla-central/rev/011dcedf181f

Comment 9

[Carsten Book [:Tomcat]]

backout completed as part of https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=494827a9190e as part of discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=937997#c48

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e18dd50141
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0bfc071cd47c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3a84277bd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01b9888de10c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1d58a7dfc9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3382fad9edf0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49229e7d1afd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/494827a9190e

however see bug 937997 for investigations going on for the root cause and tree reopening

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd7de34a6f34
https://hg.mozilla.org/mozilla-central/rev/e1e55ac4e33d

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Part 1: Add Debugger.Script.lineCount test where the script's source notes end in newline notes. (

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Part 2: Simplify and fix js_GetScriptLineExtent. (

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8341333 [details] [diff] [review]
Part 1: Add Debugger.Script.lineCount test where the script's source notes end in newline notes. (

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 754201 most recently. Not sure what introduced AutoDebugModeGC initially.

User impact if declined: 
  Firebug unusable with many tabs. Firebug has worked around this by disabling JITs permanently if it's on, which is in some ways worse since it slows down *all* tabs.

Testing completed (on m-c, etc.): 
  On m-c.

Risk to taking this patch (and alternatives if risky): 
  GC behavior changes surfacing existing leaks/OOMs. When landing on m-c new leaks were surfaced (and eventually fixed). I am nominating those fixes for uplift also.

String or IDL/UUID changes made by this patch:
  None

The Firebug slowness metabug is bug 815603. List of bugs I'm nominating for uplift in this batch:

 - bug 936143
 - bug 935228
 - bug 933882
 - bug 935470 
 - bug 942346
 - bug 934799

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8341335 [details] [diff] [review]
Part 2: Simplify and fix js_GetScriptLineExtent. (

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 754201 most recently. Not sure what introduced AutoDebugModeGC initially.

User impact if declined: 
  Firebug unusable with many tabs. Firebug has worked around this by disabling JITs permanently if it's on, which is in some ways worse since it slows down *all* tabs.

Testing completed (on m-c, etc.): 
  On m-c.

Risk to taking this patch (and alternatives if risky): 
  GC behavior changes surfacing existing leaks/OOMs. When landing on m-c new leaks were surfaced (and eventually fixed). I am nominating those fixes for uplift also.

String or IDL/UUID changes made by this patch:
  None

The Firebug slowness metabug is bug 815603. List of bugs I'm nominating for uplift in this batch:

 - bug 936143
 - bug 935228
 - bug 933882
 - bug 935470 
 - bug 942346
 - bug 934799

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8341333 [details] [diff] [review]
Part 1: Add Debugger.Script.lineCount test where the script's source notes end in newline notes. (

We can take this on Aurora and thus it will be on Beta in a week, but we're way past the point of taking untracked, major change to Firefox 26 as we are going to build our final candidate today as well as the RC.

Comment 16

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/082ef30d4b35
https://hg.mozilla.org/releases/mozilla-aurora/rev/155f0791ac67