935228 - JSD.pause and unPause can attempt to toggle debug traps on non-debug mode Baseline scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

JSD.pause and unPause promise to persist any breakpoints set on JSScripts [1], but we need to invalidate JIT code when toggling debug mode. This can get into a inconsistent state where we try to patch toggled calls on a non-debug mode BaselineScript.

Setting s-s because the patching is only guarded by an assert, and it could corrupt the instruction stream.

Comment 1

[Shu-yu Guo [:shu]]

[1] http://dxr.mozilla.org/mozilla-central/source/js/jsd/idl/jsdIDebuggerService.idl?from=jsdIDebuggerService.idl#l220

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Toggle debug traps only in debug mode. (r=?)

We need to check this because something like JSCompartment::clearTraps tries to
toggle debug traps if hasBaselineScript(), without regard for whether or not
that BL script is in debug mode.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 827821 [details] [diff] [review]
Toggle debug traps only in debug mode. (r=?)

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

Good catch.

Comment 4

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77be849d81e7

Comment 5

[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 6

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/77be849d81e7

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/fe63bf2c6a3a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

sec-high and no security approval?

Comment 9

[Andrew McCreight [:mccr8]]

I'm going to guess this affects everything aside from b2g18.

Comment 10

[Shu-yu Guo [:shu]]

Is this sec-high if it can only be caused by Firebug?

Comment 11

[Andrew McCreight [:mccr8]]

I was using the heuristic of bumping it down a level (from sec-critical) because it is Firebug related.

Is this something that can happen during the normal operation of Firebug, or something that would require a malicious Firebug-like addon?

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de3dd3c48114 landed on central

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Toggle debug traps only in debug mode. (

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> I was using the heuristic of bumping it down a level (from sec-critical)
> because it is Firebug related.
> 
> Is this something that can happen during the normal operation of Firebug, or
> something that would require a malicious Firebug-like addon?

It would require a malicious addon that uses JSD. Which... is nonexistent AFAIK.

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8341334 [details] [diff] [review]
Toggle debug traps only in debug mode. (

[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 16

[Andrew McCreight [:mccr8]]

Thanks for the explanation.  sec-moderate makes more sense then.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8341334 [details] [diff] [review]
Toggle debug traps only in debug mode. (

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 18

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/455f7d22d2fa

Comment 19

[Jan de Mooij [:jandem]]

Shu, IIUC this can be closed, right?

Comment 20

[Shu-yu Guo [:shu]]

Actually I'm not sure why it stayed open anyways, because it was REOPENED?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Shu-yu Guo [:shu] from comment #20)
> Actually I'm not sure why it stayed open anyways, because it was REOPENED?

Security bugs are resolved manually. He probably just forgot.