Closed
Bug 943925
Opened 11 years ago
Closed 11 years ago
crash in @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
People
(Reporter: jujjyl, Assigned: bjacob)
Details
(Keywords: crash, regression, Whiteboard: [native-crash])
Crash Data
Attachments
(1 file, 2 obsolete files)
(deleted),
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.
Comment 2•11 years ago
|
||
They are reindexing recent crash data. Might not be finished till EOD pacific time.
Flags: needinfo?(kbrosnan)
Reporter | ||
Comment 3•11 years ago
|
||
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*).
Updated•11 years ago
|
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
Updated•11 years ago
|
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*)
Updated•11 years ago
|
tracking-fennec: --- → ?
Whiteboard: [native-crash]
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
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*)
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
Umm, it's a Nexus 4. Why do I keep saying it's a 7? :(
Reporter | ||
Comment 9•11 years ago
|
||
Ok, sent STR instructions in an email.
Comment 10•11 years ago
|
||
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
Updated•11 years ago
|
tracking-fennec: ? → +
Reporter | ||
Comment 11•11 years ago
|
||
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.
Reporter | ||
Comment 12•11 years ago
|
||
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.
Reporter | ||
Comment 13•11 years ago
|
||
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 ].
Reporter | ||
Comment 14•11 years ago
|
||
A HTC Desire C with Android 4.0.3 does not crash.
Reporter | ||
Comment 15•11 years ago
|
||
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?
Comment 16•11 years ago
|
||
A regression window would certainly be helpful here.
Keywords: regression,
regressionwindow-wanted
Assignee | ||
Comment 17•11 years ago
|
||
There was no deliberate change to disable WebGL anywhere recently, AFAIK! That's very scary!
Comment 18•11 years ago
|
||
(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 ...
Comment 19•11 years ago
|
||
(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)
Comment 20•11 years ago
|
||
That looks like a JS issue.
Comment 21•11 years ago
|
||
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).
Updated•11 years ago
|
Crash Signature: [@ @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)] → [@ @0x0 | mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)]
[@ @0x0 | glBindBuffer ]
Comment 22•11 years ago
|
||
Is it possible to set env. variables when debugging Fennec?
Reporter | ||
Comment 23•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
Alternatively, if you have a local build you can modify, just change SignalBasedTriggersDisabled() in js/src/vm/Runtime.cpp to always return 'true'.
Assignee | ||
Comment 25•11 years ago
|
||
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).
Comment 26•11 years ago
|
||
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...
Comment 27•11 years ago
|
||
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]
Assignee | ||
Updated•11 years ago
|
Component: Canvas: WebGL → JavaScript Engine
Reporter | ||
Comment 28•11 years ago
|
||
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.
Assignee | ||
Comment 29•11 years ago
|
||
Have we sorted out whether this was a JS engine bug or not?
Flags: needinfo?(luke)
Comment 30•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Component: JavaScript Engine → Canvas: WebGL
Assignee | ||
Comment 31•11 years ago
|
||
OK, I am taking a look now.
Assignee | ||
Comment 32•11 years ago
|
||
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)
Comment 33•11 years ago
|
||
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)
Assignee | ||
Comment 34•11 years ago
|
||
(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.
Assignee | ||
Comment 35•11 years ago
|
||
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)
Comment 36•11 years ago
|
||
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)
Comment 37•11 years ago
|
||
A debug x64 build doesn't hit the assert on the demo. I'll try to repro on Android.
Assignee | ||
Comment 38•11 years ago
|
||
yay, a non-debug build of Fennec reproduces immediately on https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/10kCubes.html !
Assignee | ||
Comment 39•11 years ago
|
||
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.
Assignee | ||
Comment 40•11 years ago
|
||
#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.
Assignee | ||
Comment 41•11 years ago
|
||
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...
Assignee | ||
Comment 42•11 years ago
|
||
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)
Comment 43•11 years ago
|
||
Oups... My bad! ^^
Updated•11 years ago
|
Attachment #8358576 -
Flags: feedback+
Comment 44•11 years ago
|
||
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...
Comment 45•11 years ago
|
||
(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 46•11 years ago
|
||
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-
Assignee | ||
Comment 47•11 years ago
|
||
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 48•11 years ago
|
||
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-
Assignee | ||
Comment 49•11 years ago
|
||
Never too careful when reviewing a patch written on a Friday night ;-)
Attachment #8358686 -
Attachment is obsolete: true
Attachment #8358700 -
Flags: review?(jgilbert)
Updated•11 years ago
|
Attachment #8358700 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 50•11 years ago
|
||
Assignee: nobody → bjacob
Target Milestone: --- → mozilla29
Comment 51•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 52•11 years ago
|
||
Awesome work, thanks for this fix! My Android Nightly is now working beautifully!
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 53•11 years ago
|
||
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?
Comment 54•11 years ago
|
||
: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+
Updated•11 years ago
|
Flags: needinfo?(bjacob)
Assignee | ||
Comment 55•11 years ago
|
||
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)
Comment 56•11 years ago
|
||
(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.
Updated•11 years ago
|
status-b2g-v1.3:
--- → affected
Assignee | ||
Comment 57•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 58•11 years ago
|
||
Landed on beta. Let me know if I need to do anything still.
Comment 59•11 years ago
|
||
(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.
Assignee | ||
Comment 60•11 years ago
|
||
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.
Comment 61•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: needinfo?(bbajaj)
Comment 62•11 years ago
|
||
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.
Comment 63•11 years ago
|
||
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.
Comment 64•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(release-mgmt)
Assignee | ||
Comment 65•11 years ago
|
||
I have a proper mozilla-b2g28 clone now, so I can land this whenever you want me to.
Flags: needinfo?(bjacob)
Comment 66•11 years ago
|
||
(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
Comment 67•11 years ago
|
||
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.
Assignee | ||
Comment 68•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
Attachment #8358700 -
Flags: approval-mozilla-beta?
Updated•11 years ago
|
Attachment #8358700 -
Flags: approval-mozilla-b2g28?
Comment 69•11 years ago
|
||
(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 70•11 years ago
|
||
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)
Comment 71•11 years ago
|
||
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•