1160884 - Crash [@ js::str_split_string(JSContext*, JS::Handle<js::ObjectGroup*>, JS::Handle<JSString*>, JS::Handle<JSString*>) ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Nightly Crash report - Assertion failure: addr % CellSize == 0

1. http://www.centenario1914-1918.it/it
2. This is not reliable. You may need to reload several times.
3. Crash on Windows only so far.

bp-47d0057a-57bc-4292-8a38-58d5d2150503

exploitability: high

Also crashes debug on Beta/38, Nightly/40

Comment 1

[Bob Clary [:bc] (inactive)]

Beta 38.0b9 bp-483c463f-79d1-4143-9695-3a8472150503 same crash

Aurora 39.0a2 bp-85c87d0a-9590-4f8e-bc1b-368b02150503 
[@ JSRope::flatten(js::ExclusiveContext*) ]

Comment 2

[Bob Clary [:bc] (inactive)]

OSX 10.8 Aurora/39 gave a crash

Operating system: Mac OS X
                  10.8.5 12F2518
CPU: amd64
     family 6 model 42 stepping 7
     4 CPUs

Crash reason:  EXC_BAD_ACCESS / 0x0000000d
Crash address: 0x0
Process uptime: 16 seconds

Thread 0 (crashed)
 0  XUL!IsWriteableAddress(void*) [Nursery.cpp : 528 + 0x4]

Comment 3

[Daniel Veditz [:dveditz]]

Luke: any insights into how JS string code might be failing here?

Comment 4

[Luke Wagner [:luke]]

I haven't been in this area of the code for a pretty long time.  Randomly forwarding to Terrence since possibly GC related.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #4)
> I haven't been in this area of the code for a pretty long time.  Randomly
> forwarding to Terrence since possibly GC related.

Eh? The proximate cause here seems to be the jit calling AssertValidStringPointer on something that's not a valid pointer. Also, Jan touched strings more recently than I did, so I'm totally calling not-it on this one.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Terrence Cole [:terrence] from comment #5)
> Also, Jan touched strings more recently than I did, so I'm totally calling not-it on
> this one.

Well that was about a year ago, I really doubt this crash is a regression from that.

But somebody has to investigate this; I'll look into it in a bit.

Comment 7

[Jan de Mooij [:jandem]]

I can reproduce this with a debug+opt build on OS X, revision 7a62238aecdc. It usually crashes after reloading the URL 1-10 times. Below is the stack trace.

It's crashing under str_split_string -> ... -> Nursery::forwardBufferPointer.

That str_split_string shows up in all reports in this bug and seems a bit suspicious. Will try to find out more.

  * frame #0: 0x0000000104cf878c XUL`js::Nursery::forwardBufferPointer(js::HeapSlot**) [inlined] IsWriteableAddress(void*) at Nursery.cpp:345
    frame #1: 0x0000000104cf878c XUL`js::Nursery::forwardBufferPointer(this=<unavailable>, pSlotsElems=<unavailable>) + 124 at Nursery.cpp:372
    frame #2: 0x00000001050b3275 XUL`js::jit::UpdateIonJSFrameForMinorGC(trc=<unavailable>, frame=<unavailable>) + 437 at JitFrames.cpp:1206
    frame #3: 0x00000001050b49ba XUL`js::jit::UpdateJitActivationsForMinorGC(rt=<unavailable>, trc=0x00007fff5fbf8ed8) + 154 at JitFrames.cpp:1565
    frame #4: 0x0000000104cf8ecb XUL`js::Nursery::collect(this=0x0000000112d5c3a0, rt=0x0000000112d5c000, reason=OUT_OF_NURSERY, pretenureGroups=0x00007fff5fbf8ff8) + 1755 at Nursery.cpp:503
    frame #5: 0x00000001053060fa XUL`js::gc::GCRuntime::minorGCImpl(this=0x0000000112d5c348, reason=OUT_OF_NURSERY, pretenureGroups=0x00007fff5fbf8ff8) + 138 at jsgc.cpp:6413
    frame #6: 0x00000001053061f9 XUL`js::gc::GCRuntime::minorGC(this=0x0000000112d5c348, cx=0x0000000120d08ae0, reason=OUT_OF_NURSERY) + 137 at jsgc.cpp:6425
    frame #7: 0x0000000104d03260 XUL`JSObject* js::gc::GCRuntime::tryNewNurseryObject<(this=0x0000000112d5c348, cx=0x0000000120d08ae0, thingSize=96, nDynamicSlots=0, clasp=0x000000010723b208)1>(JSContext*, unsigned long, unsigned long, js::Class const*) + 160 at Allocator.cpp:157
    frame #8: 0x0000000104d02f0f XUL`JSObject* js::Allocate<JSObject, (cx=0x0000000120d08ae0, kind=OBJECT8_BACKGROUND, nDynamicSlots=0, heap=DefaultHeap, clasp=0x000000010723b208)1>(js::ExclusiveContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) + 175 at Allocator.cpp:123
    frame #9: 0x0000000104be61c9 XUL`js::ArrayObject::createArrayInternal(cx=0x0000000120d08ae0, kind=OBJECT8_BACKGROUND, heap=DefaultHeap, shape=<unavailable>, group=<unavailable>) + 297 at ArrayObject-inl.h:49
    frame #10: 0x0000000104be3fd9 XUL`js::ArrayObject::createArray(cx=<unavailable>, kind=OBJECT8_BACKGROUND, heap=<unavailable>, shape=<unavailable>, group=<unavailable>, length=3) + 25 at ArrayObject-inl.h:78
    frame #11: 0x0000000104bdea0a XUL`js::ArrayObject* NewArray<268435456u>(cxArg=0x0000000120d08ae0, length=3, protoArg=<unavailable>, newKind=GenericObject) + 746 at jsarray.cpp:3352
    frame #12: 0x0000000104bdef01 XUL`js::NewDenseCopiedArray(cx=<unavailable>, length=3, values=0x00007fff5fbf92f0, proto=<unavailable>, newKind=<unavailable>) + 33 at jsarray.cpp:3474
    frame #13: 0x00000001053ac072 XUL`js::ArrayObject* SplitHelper<(anonymous namespace)::SplitStringMatcher>(cx=<unavailable>, str=<unavailable>, limit=<unavailable>, splitMatch=<unavailable>, group=<unavailable>)::SplitStringMatcher const&, JS::Handle<js::ObjectGroup*>) + 658 at jsstr.cpp:3735
    frame #14: 0x00000001053ac43d XUL`js::str_split_string(cx=0x0000000120d08ae0, group=<unavailable>, str=<unavailable>, sep=<unavailable>) + 269 at jsstr.cpp:3941

Comment 8

[Jan de Mooij [:jandem]]

Reduced testcase. 32-bit debug shell asserts with --no-threads:

function foo(date) {
    var F = date.split(" ");
    var D = F[0].split("-");
    var C = F[1].split(":");
    return new Date(D[0], D[1], D[2], C[0], C[1], C[2])
}
function test() {
    with(this) {};
    for (var i = 0; i < 10000; i++)
	foo("13-5-2015 18:30:" + i);
}
test();

Comment 9

[Jan de Mooij [:jandem]]

Gary, can you run autoBisect on the shell testcase in comment 8? It crashes a 32-bit shell on OS X.

I can reproduce it on the b2g32 branch, so it probably goes pretty far back...

Comment 10

[Jan de Mooij [:jandem]]

This is bad.

The second split call here triggers a GC:

    var F = date.split(" ");
    var D = F[0].split("-");  <-----
    var C = F[1].split(":");

At this point, the LStringSplit safepoint only contains F's elements pointer, not the F object itself. So when we GC, we try to forward the elements pointer but because we didn't mark the object at all, we have no forwarding pointer at elements[0] and end up using the first element (which happens to be a string value in this case).

Then we return to JIT code, do F[1] and crash, because our elements pointer is bogus.

Unfortunately I don't see an easy/safe way to fix this on branches. bhackett, you're probably familiar with this as well since you worked on backtracking. Any thoughts?

Comment 11

[Jan de Mooij [:jandem]]

A possible fix is: all MIR (and, more importantly, LIR) instructions with a MIRType_Elements or MIRType_Slots operand, should also have an 'object' operand. It'd fix this bug, but it has downsides:

(1) It's a bit annoying to add an extra operand to these instructions. It's also easy to forget, although we could add strategic asserts to enforce this.
(2) It might affect performance: keeping the object alive longer may require an extra register or spills.

So an alternative is to have a pass right before or after Lowering, that does this:

(1) Whenever an instruction has a MIRType_Elements/MIRType_Slots operand, add a dummy MIR instruction right after it that's there just to keep the object alive.

(2) To alleviate/avoid the performance concern in the most common cases, do this only if there are non-whitelisted instructions between the MElements/MSlots and the current instruction. This way, common patterns like this won't be affected:

m1 = MSlots(obj)
m2 = MLoadSlot(m1)

Or:

m1 = MElements(obj)
m2 = MArrayLength(m1)

I think this shouldn't be too hard and seems safe enough to uplift. I can work on it this week (although tomorrow is an holiday here). Still would like to know about other ideas though.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: split patch

I think the way this avoids blowing up on us in other cases is that operations which can GC normally have a resume point afterwards.  If the object whose elements have been consolidated is still in use, it will be in that resume point and won't be collected.  The attached patch fixes this testcase, but I don't know about other places in IonBuilder/MCallOptimize where we might hit the same problem.  Fixing things this way does seem less invasive, at least.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #12)
> but I don't know about other places in IonBuilder/MCallOptimize
> where we might hit the same problem.

NewArray is similar (can GC, no resume point) and indeed I have a test that crashes under NewDenseArray. It's a bit more worrisome than StringSplit because NewArray is very common, but the extra resume point should be OK, I think.

I was a bit worried the resume point only keeps the object alive if there are actual LIR instructions with snapshots based on the RP, but fortunately there should always be an OsiPoint with a snapshot if we have a safepoint.

In LIRGeneratorShared::assignSafepoint, we have:

    MResumePoint* mrp = mir->resumePoint() ? mir->resumePoint() : lastResumePoint_;

Maybe we should just assert mir->resumePoint() here. Unfortunately this won't work for the MNewArray added in IonBuilder::inlineArray or IonBuilder::inlineConstantStringSplit, because we can't do resumeAfter there until after we store the elements. We could come up with a way to disable the assert in that case though...

I'll think about it more.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2f6de7b18305
user:        Steve Fink
date:        Mon Jan 26 15:32:54 2015 -0800
summary:     Bug 1125412 - Expose an object for inspecting GC memory values, r=terrence

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> NewArray is similar (can GC, no resume point) and indeed I have a test that
> crashes under NewDenseArray. It's a bit more worrisome than StringSplit
> because NewArray is very common, but the extra resume point should be OK, I
> think.

Adding more resume points shouldn't be a performance concern.  We actually add extra unnecessary resume points to avoid keeping dead terms alive too long, see IonBuilder::maybeInsertResume().

Comment 16

[Brian Hackett [Laid off!]]

This issue goes back further than bug 1125412.

Comment 17

[Jan de Mooij [:jandem]]

Yes, initially I thought this went back to GGC and the slots/elements forwarding I added for that (bug 888872), but even without GGC this was still a GC hazard. It'll be interesting to see if this affects crash-stats.

Comment 18

[Jan de Mooij [:jandem]]

I'm not sure if adding an extra resume point is feasible. It turns out we have a ton of these instructions with a safepoint/VM call but no resume point. To name a few common ones:

- MInterruptCheck
- MConcat
- MGetPropertyCache (idempotent ones)
- MPostWriteBarrier (this one can't actually GC but uses the safepoint to get the live regs)
- MToString
- MTruncateToInt32

We can't resumeAfter() movable instructions (see the assert in IonBuilder::resume), it will confuse LICM and other things (they don't look at resume point operands AFAICS).

MConcat and idempotent MGetPropertyCache we definitely want to move around for performance, but they *can* trigger GC. Well the idempotent GetPropertyCache maybe can't trigger GC in practice, but that's not obvious and not something I'd want to rely on.

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> This is bad.
> 
> The second split call here triggers a GC:
> 
>     var F = date.split(" ");
>     var D = F[0].split("-");  <-----
>     var C = F[1].split(":");
> 
> At this point, the LStringSplit safepoint only contains F's elements
> pointer, not the F object itself. So when we GC, we try to forward the
> elements pointer but because we didn't mark the object at all, we have no
> forwarding pointer at elements[0] and end up using the first element (which
> happens to be a string value in this case).
> 
> Then we return to JIT code, do F[1] and crash, because our elements pointer
> is bogus.

This sounds more like an alias set issue.  Where any VMFunction call should alias all GC things as the GC can relocate the elements.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #19)
> This sounds more like an alias set issue.  Where any VMFunction call should
> alias all GC things as the GC can relocate the elements.

That'd be the easiest fix, but it'd regress performance: loops have InterruptCheck instructions, so it'd no longer be possible to hoist any slots/elements outside loops.

Comment 21

[Nicolas B. Pierron [:nbp]]

If you want to keep an object alive, you should not be looking at resume points.  Resume points are only here to satisfy Baseline, nothing more.  Adding more resume points would solve the problem you are looking for in string split, because baseline expects to do a F[1], and thus keep the object alive.

Note that keeping the object alive does not mean that this object would not be relocated, and that its elements would not be poisoned.  Thus the MElement result is also invalid, if we can do a moving GC.

I think we should register MElements / MSlots as pointers-within an owned object.  Thus, in case of a safepoint, we will mark the elements / slots by marking the object.  And if the object get relocated, then we would have to update the elements / slots value as well.  Doing so will remove the alias set necessity, and will not imply that we have to add extra resume points.

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> I think we should register MElements / MSlots as pointers-within an owned
> object.  Thus, in case of a safepoint, we will mark the elements / slots by
> marking the object.  And if the object get relocated, then we would have to
> update the elements / slots value as well.  Doing so will remove the alias
> set necessity, and will not imply that we have to add extra resume points.

To go even further we could even generalized this to any pointer, where a pointer is either a GC-things or a sequence of accesses within a GC thing.  By doing so we can finally have a true LIR representation with optimizations on it, and not only a MIR clone which holds the register allocation results.

"s[i++]" could then become a pointer within the string content, as we would usually written it in C  "*s++".

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> And if the object get relocated, then we would have to
> update the elements / slots value as well.

Well this part works already; I fixed this a long time ago (bug 888872).

I'll go ahead with what I described in comment 11 (the second part).

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #23)
> Well this part works already; I fixed this a long time ago (bug 888872).

(In reply to Jan de Mooij [:jandem] from comment #11)
> (1) It's a bit annoying to add an extra operand to these instructions. It's
> also easy to forget, although we could add strategic asserts to enforce this.


Maybe the solution would be to register the input of the element / slots in the safepoint, at the same time as when we detect that we have to register the input.

This way, both the object and the element would be registered in the safepoints, as long as the element / slots are alive.  Doing it that way would not be as error-prone as adding an extra argument to all Load / Stores.

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch adds a KeepAlive instruction after instructions that use elements/slots pointers, as described in comment 11.

Fixes the crash and doesn't regress Octane/Sunspider/Kraken.

Comment 26

[Nicolas B. Pierron [:nbp]]

Attachment: WIP - add LSlots/LElements inputs into safepoints.

This is what I was thinking about, but I currently facing an assertion about the input not being registered in the liveRegs.

0x0850cfa5 in js::jit::LSafepoint::assertInvariants (this=0x8e4be28) at /home/nicolas/mozilla/wksp-3-dev/js/src/jit/LIR.h:1356
1356            MOZ_ASSERT((gcRegs().bits() & ~liveRegs().gprs().bits()) == 0);

Comment 27

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8606262 [details] [diff] [review]
Patch

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

::: js/src/jit/IonAnalysis.cpp
@@ +2824,5 @@
> +              case MDefinition::Op_StoreElement:
> +                // For typed objects, we can use these instructions with
> +                // MIRType_Object instead of MIRType_Elements input.
> +                if (ins->getOperand(0)->type() == MIRType_Object)
> +                    continue;

Wouldn't it be easier to just match for MElements / MSlots and iterate over the uses instead of filtering out all the instructions?

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #27)
> Wouldn't it be easier to just match for MElements / MSlots and iterate over
> the uses instead of filtering out all the instructions?

Good idea! That might indeed be simpler, I'll try it in a bit.

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

gkw, decoder: can you guys give this some fuzzing over the weekend?

This needs a followup patch with tests and a big comment, but let's get the fix in first.

Comment 30

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8606425 [details] [diff] [review]
Part 1 - Fix

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

::: js/src/jit/IonAnalysis.cpp
@@ +2748,5 @@
> +            return false;
> +
> +        switch (iter->op()) {
> +          case MDefinition::Op_Nop:
> +          case MDefinition::Op_Constant:

Add a comment above this switch statement, that we try to avoid adding extra pressure on registers if the sequence of instruction cannot cause the stack to be marked by any GC.

::: js/src/jit/MIR.h
@@ +8202,5 @@
>          return AliasSet::Store(AliasSet::ObjectFields);
>      }
>  };
>  
> +class MKeepAliveObject

nit: Add a comment:

  This MIR Instruction is used to register objects in safepoints, if we need to keep them alive on the stack for a GC.
  This is almost similar to what resume points are to snapshots.

Comment 33

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: use->toStoreElementHole()->object() == ownerObject, at js/src/jit/IonAnalysis.cpp:2813
Build version: mozilla-central-patch revision 127a78bac3f1+
Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug
Runtime options: --fuzzing-safe --thread-count=2 --ion-check-range-analysis --ion-offthread-compile=off --baseline-eager --ion-eager

Testcase:

var oldOpts = getJitCompilerOptions();
for (var k in oldOpts)
  setJitCompilerOption(k, oldOpts[k]);
var gTestcases = new Array();
var gTc = gTestcases.length;
function TestCase()
  gTestcases[gTc++] = this;
new TestCase();
var gTestcases = 429507;
new TestCase();
new TestCase();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000008c62ea in js::jit::AddKeepAliveInstructions (graph=...) at js/src/jit/IonAnalysis.cpp:2813
#0  0x00000000008c62ea in js::jit::AddKeepAliveInstructions (graph=...) at js/src/jit/IonAnalysis.cpp:2813
#1  0x0000000000920f1b in js::jit::OptimizeMIR (mir=mir@entry=0x7ffff69b31a8) at js/src/jit/Ion.cpp:1510
#2  0x0000000000921563 in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff69b31a8) at js/src/jit/Ion.cpp:1623
#3  0x0000000000922150 in js::jit::IonCompile (cx=cx@entry=0x7ffff691b4e0, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=js::jit::Optimization_Normal) at js/src/jit/Ion.cpp:1991
#4  0x0000000000922604 in js::jit::Compile (cx=cx@entry=0x7ffff691b4e0, script=..., script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2146
#5  0x0000000000922a86 in js::jit::CanEnter (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/jit/Ion.cpp:2298
#6  0x000000000066c64d in js::RunScript (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/vm/Interpreter.cpp:653
#7  0x000000000066cc68 in js::Invoke (cx=cx@entry=0x7ffff691b4e0, args=..., construct=construct@entry=js::CONSTRUCT) at js/src/vm/Interpreter.cpp:746
#8  0x0000000000675f27 in js::InvokeConstructor (cx=cx@entry=0x7ffff691b4e0, args=...) at js/src/vm/Interpreter.cpp:810
#9  0x0000000000676134 in js::InvokeConstructor (cx=cx@entry=0x7ffff691b4e0, fval=..., argc=argc@entry=0, argv=argv@entry=0x7fffffffcd38, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:836
#10 0x0000000000894fdf in js::jit::DoCallFallback (cx=0x7ffff691b4e0, frame=0x7fffffffcd68, stub_=<optimized out>, argc=0, vp=0x7fffffffcd28, res=...) at js/src/jit/BaselineIC.cpp:10410
#11 0x00007ffff7feea4f in ?? ()
[...]
#21 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69b58c8	140737330763976
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffb930	140737488337200
rsp	0x7fffffffb8c0	140737488337088
r8	0x7ffff7fe0780	140737354008448
r9	0x6568637461702d6c	7307199746910727532
r10	0x7fffffffb680	140737488336512
r11	0x7ffff6c27960	140737333328224
r12	0x7ffff69b5918	140737330764056
r13	0x7ffff69b4c98	140737330760856
r14	0x7ffff69b5958	140737330764120
r15	0x7ffff69b3040	140737330753600
rip	0x8c62ea <js::jit::AddKeepAliveInstructions(js::jit::MIRGraph&)+1146>
=> 0x8c62ea <js::jit::AddKeepAliveInstructions(js::jit::MIRGraph&)+1146>:	movl   $0xafd,0x0
   0x8c62f5 <js::jit::AddKeepAliveInstructions(js::jit::MIRGraph&)+1157>:	callq  0x4907d0 <abort()>

Comment 34

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix v2

@decoder: thanks! The assert is bogus when GVN is disabled (this also revealed a bug in the getJitCompilerOptions function; will file a separate bug).

@nbp: will add the comments as part of the second part :)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Staring at the patch for a while should make it clear what the issue is. Going from that to an exploit or testcase is not trivial but for somebody with some JIT compiler experience it's also not rocket science...

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Shouldn't be too hard to backport.

How likely is this patch to cause regressions; how much testing does it need?

Fuzz testing didn't reveal any (serious) issues, and the fix/algorithm isn't very complicated. IMO leaving this unfixed is a bigger risk.

Comment 35

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

Sec-approval+ for checking this in on May 26 or later.

We should fix this in branches after it is checked into trunk as well.

Comment 36

[Jan de Mooij [:jandem]]

NI myself to land this today.

Comment 37

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da2e29afa891

Keeping the needinfo to uplift this after it has been in a Nightly or two.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Why is this marked leave-open?

Comment 39

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> Why is this marked leave-open?

I still have to land a test, comments and some more asserts after this is on all branches.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

(In reply to Jan de Mooij [:jandem] from comment #39)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> > Why is this marked leave-open?
> 
> I still have to land a test, comments and some more asserts after this is on
> all branches.

Might that be better for a follow-up bug? Would simplify tracking what fixes landed where at least.

Comment 41

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> Might that be better for a follow-up bug? Would simplify tracking what fixes
> landed where at least.

OK, WFM.

Comment 42

[Jan de Mooij [:jandem]]

Sigh, stupid NI clearing.

Comment 43

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Jan de Mooij [:jandem] from comment #29)
> gkw, decoder: can you guys give this some fuzzing over the weekend?

I didn't get to this due to PTO but it seems it got some fuzzing from decoder already, punting to that.

Comment 44

[Jan de Mooij [:jandem]]

Comment on attachment 8607019 [details] [diff] [review]
Part 1 - Fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown, old bug.
User impact if declined: Crashes, security bugs.
Testing completed: On m-c for 5 days.
Risk to taking this patch (and alternatives if risky): Low. Might cause minor perf regressions.
String or UUID changes made by this patch: None.

Comment 45

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66b5afbbe9a7
https://hg.mozilla.org/releases/mozilla-beta/rev/6d8c0c9dc553

Comment 46

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/266dc3dff739
https://hg.mozilla.org/releases/mozilla-esr38/rev/2c90c279e3a7

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/085bbf278601
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b15caf5da273
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2cd0112f2e3c

Ryan, can you confirm I got everything? Thanks.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Looks exactly right, thanks!

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b15caf5da273
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/085bbf278601

Comment 48

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash using testcase from comment 8 on Firefox 38 beta 8 under Windows 7 64-bit.
pb-453db188-c381-4a71-b286-508732150701

Verified that the issue is fixed using Firefox 31.8.0 ESR and 38.1.0 ESR on Windows 7 64-bit, Ubuntu 13.04 64-bit and Mac OS X 10.10.

Comment 49

[Jan de Mooij [:jandem]]

Added the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae197516f622

Comment 50

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

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