1136397 - [jsdbg2] Crash after resuming from breakpoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

STR:

1) Turn off parallel parsing in about:config to work around existing stupid problems in setting breakpoints.

2) Go to http://playcanv.as/p/JtL2iqIH and wait for it to load.

3) Find game.js, set a bp on line 158 inside setFuel.

4) Reload, wait for pause, then resume.

Socorro shows crash inside EnterBaselineAtBranch, like other crashes recently. I'll look into this promptly.

Comment 1

[Shu-yu Guo [:shu]]

Setting sec-moderate as this can only be triggered when using the frontend debugger.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Ensure OSR frame scripts have debug instrumentation.

Jan, the explanation is in the bug. This is pretty bad, but luckily only
triggerable when debugging. It should also be backported post haste.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8568878 [details] [diff] [review]
Ensure OSR frame scripts have debug instrumentation.

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

Looks good, great catch!

It will be interesting to watch crash-stats after this lands.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8568878 [details] [diff] [review]
Ensure OSR frame scripts have debug instrumentation.

As a sec-moderate, this can just go in. Sec-approval is only needed for sec-high and sec-critical issues.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8568878 [details] [diff] [review]
Ensure OSR frame scripts have debug instrumentation.

Approval Request Comment
[Feature/regressing bug #]: 1032869
[User impact if declined]: Crashing when using the Debugger
[Describe test coverage new/current, TreeHerder]: on inbound
[Risks and why]: Low? Fixes a frequent crasher and security bug, but the patch paints a pretty clear picture of how to exploit the bug, though the exploit requires explicit user interaction with the frontend JS debugger.
[String/UUID change made/needed]: None

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

As Al said, as a sec-moderate bug, we can check this in on m-c. Let's verify the fix there before uplifting to Aurora and Beta.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #6)
> As Al said, as a sec-moderate bug, we can check this in on m-c. Let's verify
> the fix there before uplifting to Aurora and Beta.

How do we verify the fix? The biggest tell is if crashes on Socorro go down, and most of those crashes came from Aurora, thus the uplifts.

Comment 8

[Shu-yu Guo [:shu]]

This was merged, btw: https://hg.mozilla.org/mozilla-central/rev/da80a09d5b4e

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Shu-yu Guo [:shu] from comment #7)
> How do we verify the fix? The biggest tell is if crashes on Socorro go down,
> and most of those crashes came from Aurora, thus the uplifts.

Typically we look to see if the signature is present on Nightly before the change is committed and, after a few days, see if any new reports with the signature are present on Nightly.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #9)
> Typically we look to see if the signature is present on Nightly before the
> change is committed and, after a few days, see if any new reports with the
> signature are present on Nightly.

Shu was able to reproduce the debugger crash on PlayCanvas and verified the patch fixes this crash. I agree we should keep an eye on crash-stats, but the patch is known to fix at least some crashes so I think there's a good reason to backport right? :)

(And the other crashes look very similar to this one..)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I'm a little lost as to why this bug wasn't resolved when it landed on m-c, but oh well.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8568878 [details] [diff] [review]
Ensure OSR frame scripts have debug instrumentation.

(In reply to Jan de Mooij [:jandem] from comment #10)
> Shu was able to reproduce the debugger crash on PlayCanvas and verified the
> patch fixes this crash. I agree we should keep an eye on crash-stats, but
> the patch is known to fix at least some crashes so I think there's a good
> reason to backport right? :)

Yes. I think this is a good argument for backporting. I missed that this was easily reproducible and didn't see any indication that the fix had been verified. Let's take this fix in Beta 3. Beta+ Aurora+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ef3b3c12db9
https://hg.mozilla.org/releases/mozilla-beta/rev/666a1aafecfd