Closed Bug 943925 Opened 11 years ago Closed 10 years ago

crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
28 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
fennec + ---

People

(Reporter: jujjyl, Assigned: bjacob)

Details

(Keywords: crash, regression, Whiteboard: [native-crash])

Crash Data

Attachments

(1 file, 2 obsolete files)

fix-drawbuffers
2.90 KB, patch
jgilbert
: review+
bajaj
: approval-mozilla-beta+
Details | Diff | Splinter Review 
As of today, Nightly started crashing on me when running a development version of an Emscripten-compiled game on a Nexus 4 Android device.

Firefox generated the following crash reports:
https://crash-stats.mozilla.com/report/index/bp-a8283715-8224-4b78-8400-f22c02131127
https://crash-stats.mozilla.com/report/index/bp-f76de0ad-712d-4015-9609-40d9e2131127
https://crash-stats.mozilla.com/report/index/bp-d6ae30be-680d-4328-8e45-ab96a2131127
https://crash-stats.mozilla.com/report/index/bp-b30c48d5-0200-4252-abc6-cde1c2131127

but I am unable to access any of those.

Contact me to query for a build that reproduces it, as it is non-public Mozilla partner code. Same code works ok on current stable Firefox for Android and OSX desktop Firefox.
Kevin do you have access to those reports?
Flags: needinfo?(kbrosnan)
They are reindexing recent crash data. Might not be finished till EOD pacific time.
Flags: needinfo?(kbrosnan)
I was able to access these crash reports today, and it looks like a WebGL bug with null pointer access in mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*).
Crash Signature: [@ @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)]
Component: General → Canvas: WebGL
Keywords: crash
Product: Firefox for Android → Core
Summary: Emscripten-based game crashes on Nightly. → 3 of the 4 are that crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)
Version: Firefox 28 → 28 Branch
Summary: 3 of the 4 are that crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*) → crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)
tracking-fennec: --- → ?
Whiteboard: [native-crash]
Looks like this signature has so far only happened on your Nexus 7 device and it's running KitKat (Android 4.4)
Summary: crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*) → crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*) (Nexus 4, KK)
I would not assume that this is device specific so quickly.

Jukka we would need access to the code that causes the issue to look into this. The data can be kept private should that be needed.
Summary: crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*) (Nexus 4, KK) → crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)
(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> I would not assume that this is device specific so quickly.

Well, so far I would not even assume that it happens on a different Nexus 7 than Jukka's. ;-)
I still found it interesting enough that this is another case of a crash with KitKat on a Nexus-line device that already has that update. But then, it's Nightly, so any number of things could be going on.
Umm, it's a Nexus 4. Why do I keep saying it's a 7? :(
What are the STR for this?
Ok, sent STR instructions in an email.
Another reproducible (public) testcase I hit on my Nexus 4 (running Android 4.4) is any of the 3 "Emscripten" demos on http://www.flohofwoe.net/demos.html
tracking-fennec: ? → +
Also, found out that the following publicly accessible link reproduces the same crash on FF Nightly on Android:

https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/10kCubes.html

This is probably better to use than the non-public Mozilla partner code.
A Nexus 10 tablet with Android 4.3 crashes also, tested with Aurora installed today:

https://crash-stats.mozilla.com/report/index/bef989a9-408e-4eb7-b388-ff3662131216

Note that the call stack of this crash differs from the others I tested with a Nexus 4: this one reads [@ @0x0 | glBindBuffer ].
A HTC Desire C with Android 4.0.3 does not crash.
I notice while installing Firefox Nightly to different Android devices (HTC Desire C, Nexus 4, Nexus 10) that FF Nightly now defaults to WebGL being turned OFF, but Aurora, Beta and Stable default to WebGL being on. I could not find any information about this. Was this change deliberate?
A regression window would certainly be helpful here.
Keywords: regression, regressionwindow-wanted
There was no deliberate change to disable WebGL anywhere recently, AFAIK! That's very scary!
(In reply to Jukka Jylänki from comment #12)
> Downloaded Aurora and Beta today to the Nexus 4 phone and tried
> https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/
> 10kCubes.html
> 
> Aurora crashes:
> https://crash-stats.mozilla.com/report/index/ca99c1a8-cac5-468d-8130-
> ebaff2131216
> https://crash-stats.mozilla.com/report/index/12f05996-07a6-4b42-b51f-
> 0f2452131216
> 
> Beta does not crash, and runs ok.

I've been investigating this and it's really odd ...
(In reply to Jukka Jylänki from comment #12)
> Downloaded Aurora and Beta today to the Nexus 4 phone and tried
> https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/
> 10kCubes.html
> 
> Aurora crashes:
> https://crash-stats.mozilla.com/report/index/ca99c1a8-cac5-468d-8130-
> ebaff2131216
> https://crash-stats.mozilla.com/report/index/12f05996-07a6-4b42-b51f-
> 0f2452131216
> 
> Beta does not crash, and runs ok.

I installed jimdb and see the following in both optimised and debug builds of fennec.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 9479]
0x822a0ab2 in js::jit::Assembler::getPtr32Target<js::jit::InstructionIterator> (start=0x7769d680, dest=0x7769d67c, style=0x7769d678)
    at /Users/djg/Mozilla/hg/mozilla-central/js/src/jit/arm/Assembler-arm.cpp:767
767	    MOZ_ASSUME_UNREACHABLE("unsupported relocation");
Flags: needinfo?(jujjyl)
That looks like a JS issue.
That SIGSEGV isn't fatal; it's about to be handled.  Try running the debugger again with JS_DISABLE_SLOW_SCRIPT_SIGNALS=1 environment variable (see https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb for more details on why).
Crash Signature: [@ @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)] → [@ @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)] [@ @0x0 | glBindBuffer ]
Is it possible to set env. variables when debugging Fennec?
Does this help? https://wiki.mozilla.org/Mobile/Fennec/Android : "If you need to set an environment variable at run time, append --es env# VAR=VAL to your activity manager command where # is the ordered number of variables"
Flags: needinfo?(jujjyl)
Alternatively, if you have a local build you can modify, just change SignalBasedTriggersDisabled() in js/src/vm/Runtime.cpp to always return 'true'.
More simply, if you have jimdb properly set up ( https://wiki.mozilla.org/Mobile/Fennec/Android/GDB ), just running jimdb will prompt you if you want to add any variables to the environment.

(Hint: it is very much worth setting up jimdb properly).
I changed the code in js/src/vm/Runtime.cpp return true.

I still see this:

adb| [15226] WARNING: NS_ENSURE_TRUE(siteSpecificUA) failed: file /Users/djg/Mozilla/hg/mozilla-central/dom/base/Navigator.cpp, line 297

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 15261]
0x7d80711e in js::jit::Assembler::getPtr32Target<js::jit::InstructionIterator> (start=<optimized out>, dest=0x7705f3c8, style=0x7705f3cc)
    at /Users/djg/Mozilla/hg/mozilla-central/js/src/jit/arm/Assembler-arm.cpp:767
767	    MOZ_ASSUME_UNREACHABLE("unsupported relocation");
(gdb) s
adb| Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 15261 (Gecko)
adb| Using new thumbnail size: 280000 (width 350)
adb| Sending thumbnail event: 350, 200
Remote connection closed
(gdb)

I might have to defer to a Fennec expert...
Sometimes I get a little bit further:Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 18360]
0x7d29111e in js::jit::Assembler::getPtr32Target<js::jit::InstructionIterator> (start=<optimized out>, dest=0x76f5f740, style=0x76f5f744)
    at /Users/djg/Mozilla/hg/mozilla-central/js/src/jit/arm/Assembler-arm.cpp:767
767	    MOZ_ASSUME_UNREACHABLE("unsupported relocation");
(gdb) s
adb| Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 18360 (Gecko)
adb| Using new thumbnail size: 280000 (width 350)
adb| Sending thumbnail event: 350, 200
adb| onTabChanged: FAVICON
adb| setFavicon(android.graphics.Bitmap@41f081f0)
adb| BrowserApp.onTabChanged: 0: FAVICON
nsProfileLock::FatalSignalHandler (signo=11, info=0x76a49c90, context=0x76a49d10)
    at /Users/djg/Mozilla/hg/mozilla-central/profile/dirserviceprovider/src/nsProfileLock.cpp:195
195	    _exit(signo);
(gdb) bt
#0  nsProfileLock::FatalSignalHandler (signo=11, info=0x76a49c90, context=0x76a49d10)
    at /Users/djg/Mozilla/hg/mozilla-central/profile/dirserviceprovider/src/nsProfileLock.cpp:195
#1  0x7d14b09a in AsmJSFaultHandler (context=0x76a49d10, info=0x76a49c90, signum=11)
    at /Users/djg/Mozilla/hg/mozilla-central/js/src/jit/AsmJSSignalHandlers.cpp:931
#2  AsmJSFaultHandler (signum=11, info=0x76a49c90, context=0x76a49d10) at /Users/djg/Mozilla/hg/mozilla-central/js/src/jit/AsmJSSignalHandlers.cpp:913
#3  0x7519d73a in SEGVHandler::handler (signum=11, info=0x76a49c90, context=0x76a49d10)
    at /Users/djg/Mozilla/hg/mozilla-central/mozglue/linker/ElfLoader.cpp:1095
#4  0xffff0514 in ?? ()
#5  0xffff0514 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) s
[Inferior 1 (process 18339) exited with code 013]
Component: Canvas: WebGL → JavaScript Engine
Any updates on this is going? I'm somewhat blocked from doing any work on Android at the moment and it's becoming a bit critical.
Have we sorted out whether this was a JS engine bug or not?
Flags: needinfo?(luke)
Not a JS engine bug; earlier comments were confused by the asm.js/Ion/ElfHack signal handler tricks.  I was able to hit the ForceClearFramebufferWithDefaultValues crash in gdb a few days ago and there isn't any JS on the stack.
Flags: needinfo?(luke)
Component: JavaScript Engine → Canvas: WebGL
OK, I am taking a look now.
So far I am only hitting this bug. Luke, can you help me interprete it?

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 4524]
0x7e329f2c in back (this=<optimized out>)
    at ../../dist/include/mozilla/Vector.h:384
384           MOZ_ASSERT(!entered);
(gdb) bt
#0  0x7e329f2c in back (this=<optimized out>)
    at ../../dist/include/mozilla/Vector.h:384
#1  start (this=<optimized out>)
    at /hack/mozilla-central/js/src/jit/LiveRangeAllocator.h:268
#2  findFirstSafepoint (startFrom=<optimized out>, 
    interval=<optimized out>, this=<optimized out>)
    at /hack/mozilla-central/js/src/jit/LiveRangeAllocator.h:721
#3  js::jit::LinearScanAllocator::populateSafepoints (
    this=this@entry=0x77365048)
    at /hack/mozilla-central/js/src/jit/LinearScan.cpp:499
#4  0x7e332ff4 in js::jit::LinearScanAllocator::go (
    this=this@entry=0x77365048)
    at /hack/mozilla-central/js/src/jit/LinearScan.cpp:1272
#5  0x7e33317c in js::jit::GenerateLIR (mir=mir@entry=0x8625f0e8)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1436
#6  0x7e33fc62 in js::jit::CompileBackEnd (mir=mir@entry=0x8625f0e8, 
    maybeMasm=maybeMasm@entry=0x0)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1527
#7  0x7e3402fc in IonCompile (
    optimizationLevel=js::jit::Optimization_Normal, recompile=false, 
    executionMode=js::SequentialExecution, constructing=false, osrPc=0x0, 
    baselineFrame=<optimized out>, script=0x77c1c800, cx=0x86645120)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1769
#8  js::jit::Compile (cx=cx@entry=0x86645120, script=script@entry=..., 
    osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, 
    constructing=false, 
    executionMode=executionMode@entry=js::SequentialExecution)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1972
#9  0x7e340754 in js::jit::CanEnter (cx=0x86645120, state=...)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:2110
#10 0x7e4d25ee in RunScript (state=..., cx=0x86645120)
    at /hack/mozilla-central/js/src/vm/Interpreter.cpp:395
#11 js::RunScript (cx=0x86645120, state=...)
    at /hack/mozilla-central/js/src/vm/Interpreter.cpp:387
#12 0x7e4d3a46 in js::Invoke (cx=cx@entry=0x86645120, args=..., 
    construct=construct@entry=js::NO_CONSTRUCT)
    at /hack/mozilla-central/js/src/vm/Interpreter.cpp:482
#13 0x7e4d3fe8 in js::Invoke (cx=0x86645120, thisv=..., fval=..., argc=4, 
    argv=0x773660f0, rval=...)
    at /hack/mozilla-central/js/src/vm/Interpreter.cpp:519
#14 0x7e265980 in js::InvokeFromAsmJS_Ignore (cx=0x86645120, 
    exitIndex=116, argc=4, argv=0x773660f0)
    at /hack/mozilla-central/js/src/jit/AsmJS.cpp:5970
#15 0x8d1fbacc in ?? ()
Flags: needinfo?(luke)
Are you running a tip build with JS_DISABLE_SLOW_SCRIPT_SIGNALS=1?  Is the SIGSEGV the assert itself or accessing the memory to evaluate the expression being asserted?  Is this running https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/10kCubes.html ?
Flags: needinfo?(luke)
(In reply to Luke Wagner [:luke] from comment #33)
> Are you running a tip build with JS_DISABLE_SLOW_SCRIPT_SIGNALS=1?

A tip build but without any particular env var defined.

I might have set the pref to disable slow script warnings.

> Is the
> SIGSEGV the assert itself or accessing the memory to evaluate the expression
> being asserted?

I'll try to figure that the next time; making a build with js/src rebuilt with -O0 -g3 at the moment.

>  Is this running
> https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/
> 10kCubes.html ?

Yes.
With js/src rebuild with -O0 -g3, I got this crash:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 5279]
0x7e6d56b2 in js::jit::LinearScanAllocator::populateSafepoints (
    this=0x8687f9d0) at /hack/mozilla-central/js/src/jit/LinearScan.cpp:595
595                         JS_ASSERT(typeAlloc->isArgument());
(gdb) bt
#0  0x7e6d56b2 in js::jit::LinearScanAllocator::populateSafepoints (
    this=0x8687f9d0) at /hack/mozilla-central/js/src/jit/LinearScan.cpp:595
#1  0x7e6d8418 in js::jit::LinearScanAllocator::go (this=0x8687f9d0)
    at /hack/mozilla-central/js/src/jit/LinearScan.cpp:1272
#2  0x7e6950bc in js::jit::GenerateLIR (mir=0x86a0f0e8)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1436
#3  0x7e69536a in js::jit::CompileBackEnd (mir=0x86a0f0e8, maybeMasm=0x0)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1527
#4  0x7e8ccafc in js::WorkerThread::handleIonWorkload (this=0x83794d98, 
    state=...) at /hack/mozilla-central/js/src/jsworkers.cpp:785
#5  0x7e8cd530 in js::WorkerThread::threadLoop (this=0x83794d98)
    at /hack/mozilla-central/js/src/jsworkers.cpp:1024
#6  0x7e8cc73c in js::WorkerThread::ThreadMain (arg=0x83794d98)
    at /hack/mozilla-central/js/src/jsworkers.cpp:704
#7  0x7979bd78 in _pt_root (arg=0x864e0380)
    at /hack/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:205
#8  0x4007ca5c in __thread_entry ()
   from /hack/jimdb/lib/R32CB05R7HN/system/lib/libc.so
#9  0x4007cbd8 in pthread_create ()
   from /hack/jimdb/lib/R32CB05R7HN/system/lib/libc.so
#10 0x00000000 in ?? ()
(gdb) p typeAlloc
$1 = (js::jit::LAllocation *) 0x8790a508
(gdb) set print pretty on
(gdb) p *typeAlloc
$2 = {
  <js::jit::TempObject> = {<No data fields>}, 
  members of js::jit::LAllocation: 
  bits_ = 6, 
  static TAG_BIT = 1, 
  static TAG_SHIFT = 0, 
  static TAG_MASK = 1, 
  static KIND_BITS = 3, 
  static KIND_SHIFT = 1, 
  static KIND_MASK = 7, 
  static DATA_BITS = 28, 
  static DATA_SHIFT = 4, 
  static DATA_MASK = 268435455
}
(gdb) l
590                     // safepoint entry, as long as the vreg does not have a stack
591                     // slot as canonical spill slot.
592                     if (payloadAlloc->isArgument() &&
593                         (!payload->canonicalSpill() || payload->canonicalSpill() == payloadAlloc))
594                     {
595                         JS_ASSERT(typeAlloc->isArgument());
596                         JS_ASSERT(!type->canonicalSpill() || type->canonicalSpill() == typeAlloc);
597                         continue;
598                     }
599
(gdb) p typeAlloc->isArgument()
$3 = false


I still have the GDB session open, let me know if you want more information from it.
Flags: needinfo?(luke)
Hmm, this is possibly a recent regression.  Marty/Douglas: anything recent regalloc changes on ARM?  I'll check to see whether this asserts in a debug x86 build.

In the meantime, perhaps you could take a quick look at the ForceClearFramebufferWithDefaultValues crash in opt builds to see if anything sticks out?
Flags: needinfo?(luke)
A debug x64 build doesn't hit the assert on the demo.  I'll try to repro on Android.
yay, a non-debug build of Fennec reproduces immediately on https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/10kCubes.html !
Dang, there is something really stupid going on here. We're using glDrawBuffers, which is not always available (depends on MRT extensions), unconditionally. We crash as that function pointer is null when it isn't available.
#1  0x7c519510 in mozilla::gl::GLContext::fDrawBuffers (this=0x84c1fc00, 
    n=4, bufs=0x773dbf84) at /hack/mozilla-central/gfx/gl/GLContext.h:2120
#2  0x7c9aa34c in mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues (this=0x8110a2f0, mask=16640, colorAttachmentsMask=0x773dbffc)
    at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:1107
#3  0x7c9aa1f4 in mozilla::WebGLContext::ClearScreen (this=0x8110a2f0)
    at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:1000

The GLContext here is perfectly fine, is not in 'destroyed' state, regular GL entry points are initialized, just fDrawBuffers is nullptr because that GL driver doesn't support it.
Wait, it's more funny than that. We're only calling fDrawBuffers if 'drawBuffersIsEnabled' is true, which it is here. Which means that the WEBGL_draw_buffers extension is enabled. That shouldn't happen if the fDrawBuffers function pointer is null. Investigating more...
We were loading draw_buffers symbols as part of the desktop-GL-only symbols! Which explains why we didn't load them on mobile devices, even when the GL extensions were available.
Attachment #8358576 - Flags: review?(jgilbert)
Oups... My bad! ^^
Attachment #8358576 - Flags: feedback+
Looks like someone just filed a bug for the isArgument() assert you were seeing: bug 958732.  Seems like automated testing for debug ARM builds is spotty...
(In reply to Luke Wagner [:luke] from comment #44)
> Looks like someone just filed a bug for the isArgument() assert you were
> seeing: bug 958732.  Seems like automated testing for debug ARM builds is
> spotty...

'Spotty' is a polite way to put it, yes. :p
Comment on attachment 8358576 [details] [diff] [review]
Properly load draw_buffers symbols

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

::: gfx/gl/GLContext.cpp
@@ -491,5 @@
>                  { (PRFuncPtr*) &mSymbols.fReadBuffer, { "ReadBuffer", nullptr } },
>                  { (PRFuncPtr*) &mSymbols.fMapBuffer, { "MapBuffer", nullptr } },
>                  { (PRFuncPtr*) &mSymbols.fUnmapBuffer, { "UnmapBuffer", nullptr } },
>                  { (PRFuncPtr*) &mSymbols.fPointParameterf, { "PointParameterf", nullptr } },
> -                { (PRFuncPtr*) &mSymbols.fDrawBuffer, { "DrawBuffer", nullptr } },

`DrawBuffer` is desktop GL only. `DrawBuffers` is the cross-platform one.
Attachment #8358576 - Flags: review?(jgilbert) → review-
Attached patch Properly load fDrawBuffers (obsolete) — — Splinter Review 
Ah, I didn't really know what fDrawBuffer was. Good catch. How about this?
Attachment #8358576 - Attachment is obsolete: true
Attachment #8358686 - Flags: review?(jgilbert)
Comment on attachment 8358686 [details] [diff] [review]
Properly load fDrawBuffers

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

::: gfx/gl/GLContext.cpp
@@ +986,5 @@
> +            if (!LoadSymbols(drawBuffersSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports draw_buffers without supplying its functions.");
> +
> +                MarkUnsupported(GLFeature::draw_buffers);
> +                mSymbols.fDrawBuffer = nullptr;

You forgot to remove this line.
Attachment #8358686 - Flags: review?(jgilbert) → review-
Attached patch fix-drawbuffers — — Splinter Review 
Never too careful when reviewing a patch written on a Friday night ;-)
Attachment #8358686 - Attachment is obsolete: true
Attachment #8358700 - Flags: review?(jgilbert)
Attachment #8358700 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d32a34f7011
Assignee: nobody → bjacob
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/1d32a34f7011
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Awesome work, thanks for this fix! My Android Nightly is now working beautifully!
We need this on 1.3. This blocks a preinstalled partner app from working correctly on 1.3, as we'll crash on startup of that app. I've confirmed this works on trunk with the app on 1.4.
blocking-b2g: --- → 1.3?
:bjacob can you please help with the risk profile here and uplift it to beta branch ? that way the b2g branch will get this fix too.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(bjacob)
The risk is very low: this is a small patch of mostly boilerplate code and has 1) been on central for a month and 2) been confirmed to fix the bug.
Flags: needinfo?(bjacob)
(In reply to Benoit Jacob [:bjacob] from comment #55)
> The risk is very low: this is a small patch of mostly boilerplate code and
> has 1) been on central for a month and 2) been confirmed to fix the bug.

great thanks, lets get this uplifted to beta then given this is Fx28 regression.
status-firefox27: --- → unaffected
status-firefox28: --- → affected
status-firefox29: --- → fixed
Landed on beta. Let me know if I need to do anything still.
(In reply to Benoit Jacob [:bjacob] from comment #58)
> Landed on beta. Let me know if I need to do anything still.

Well actually not Beta here, you need to land that on the b2g28 branch.
Sorry, I mis-interpreted comment 54 then. I'll get a b2g28 tree then. Meanwhile, if there is a possibility of getting this auto-landed for me, I'm interested :-) The patch that landed on beta (comment 57) will apply cleanly.
Wow, this landing was all sorts of wrong.
1) 1.3 blockers are not landing on beta, they're landing on b2g28.
2) 1.3 blockers need *explicit* approval to land on b2g28 at this point. Having blocking status is NOT automatic approval to land at this point.

See https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3.0

At this point, your best bet is to get retro-active beta approval for this patch and let it auto-merge over to b2g28. Or back this out from beta and go through the proper b2g28 approval process as should have been done in the first place.
Flags: needinfo?(bjacob)
Flags: needinfo?(bbajaj)
bbajaj for the record verbally did approve of this patch. Sorry if that wasn't clear - I should have transcribed the conversation in person to the bug.
That said, Firefox for Android can need it on Beta as well, and beta is being automatically merged to b2g28 anyhow from what I understand. If we leave this in the current state, things are probably fine. We should try doing things in more clarity and order next time, though.
FWIW, given the new restrictions on what lands on b2g28, I'm not comfortable merging beta to b2g28 until this is formally approved by RelMan.
Flags: needinfo?(release-mgmt)
I have a proper mozilla-b2g28 clone now, so I can land this whenever you want me to.
Flags: needinfo?(bjacob)
(In reply to Benoit Jacob [:bjacob] from comment #65)
> I have a proper mozilla-b2g28 clone now, so I can land this whenever you
> want me to.

Note - as Ryan suggests, we need to fill out the approval form here & get approval before we land this.

See https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4
As has been requested multiple times now, just get beta approval for the landing you already did and this will be merged to b2g28. Per comment 64, this matter is currently holding up this bug and many others from being merged to b2g28.

There is absolutely no reason to get b2g28 approval and land it directly there at this point. Please just get the beta approval so we can do the merge.
Comment on attachment 8358700 [details] [diff] [review]
fix-drawbuffers

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 843667 (i.e. ever since we supported WEBGL_draw_buffers)
User impact if declined: Applications using WEBGL_draw_buffers (i.e. some 3D games) crash on mobile devices
Testing completed: Been on m-c for a month, and on beta since Friday
Risk to taking this patch (and alternatives if risky): very low risk as said above, small mostly-boilerplate patch
String or UUID changes made by this patch: none
Attachment #8358700 - Flags: approval-mozilla-b2g28?
Attachment #8358700 - Flags: approval-mozilla-beta?
Attachment #8358700 - Flags: approval-mozilla-b2g28?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #67)
> There is absolutely no reason to get b2g28 approval and land it directly
> there at this point. Please just get the beta approval so we can do the
> merge.
Comment on attachment 8358700 [details] [diff] [review]
fix-drawbuffers

Doing the retro active approval here to clear this up. Ideally what I meant by uplift in https://bugzilla.mozilla.org/show_bug.cgi?id=943925#c56 was to request for beta approval which would them get auto-merged to b2g28. Looks like there was some confusion there, I'll be more explicit next time :)
Attachment #8358700 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(release-mgmt)
Flags: needinfo?(bbajaj)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: