Last Comment Bug 778765 - crash at [@ libc-2.15.so@0x36445 ] at MapsGL with webgl.msaa-level = 1
: crash at [@ libc-2.15.so@0x36445 ] at MapsGL with webgl.msaa-level = 1
Status: VERIFIED FIXED
[advisory-tracking+][qa-]
: crash, sec-moderate
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All Linux
: -- critical (vote)
: mozilla17
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
https://maps.google.com/?vector=1
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 09:37 PDT by Daniel Holbert [:dholbert]
Modified: 2012-10-21 22:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
verified
15+
fixed


Attachments
block MSAA on nouveau (1.34 KB, patch)
2012-07-30 11:31 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
samples: change 1 into 0 (793 bytes, patch)
2012-08-02 13:49 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-07-30 09:37:15 PDT
STR:
 1. Visit about:config
 2. Set the pref webgl.msaa-level = 1
     (I didn't do this manually, at first -- this was already set in my profile for
      some reason. Not sure why.)

 3. Visit https://maps.google.com/?vector=1  (which should enable MapsGL)

ACTUAL RESULTS: Crash, with this printed to my terminal:
firefox: nvc0_surface.c:218: nvc0_resource_copy_region: Assertion `src->nr_samples == dst->nr_samples' failed.

Sample crash reports:
 bp-63b77e54-c5e4-45c6-b933-c26a32120730
 bp-5ec26eac-8033-4661-977d-b52222120730
Comment 1 Daniel Holbert [:dholbert] 2012-07-30 09:39:36 PDT
about:support info:

User Agent
   Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0

GRAPHICS:
Adapter Description
   nouveau -- Gallium 0.4 on NVC1
Vendor ID
   nouveau
Device ID
   Gallium 0.4 on NVC1
Driver Version
   3.0 Mesa 8.0.3
WebGL Renderer
   nouveau -- Gallium 0.4 on NVC1 -- 3.0 Mesa 8.0.3
GPU Accelerated Windows
   0
AzureBackend
   none
Comment 2 Daniel Holbert [:dholbert] 2012-07-30 09:41:06 PDT
Flagging as security-sensitive because this is a crash at a random-ish address (e.g. 0x3e80000175f in the first crash report from comment 0) that sometimes happens inside of free() (in the second crash report from comment 0)
Comment 3 Daniel Holbert [:dholbert] 2012-07-30 09:42:16 PDT
I'm running Ubuntu 12.10 alpha 3 as my OS, too, BTW.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-07-30 09:57:27 PDT
Jeff, should we blacklist msaa on the Nouveau driver, or do you see a workaround from the assert message,

firefox: nvc0_surface.c:218: nvc0_resource_copy_region: Assertion `src->nr_samples == dst->nr_samples' failed.

?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-07-30 11:31:17 PDT
Created attachment 647224 [details] [diff] [review]
block MSAA on nouveau
Comment 6 Jeff Gilbert [:jgilbert] 2012-07-30 13:11:30 PDT
(In reply to Daniel Holbert [:dholbert] (away from 7/28-8/5) from comment #0)
> STR:
>  1. Visit about:config
>  2. Set the pref webgl.msaa-level = 1
>      (I didn't do this manually, at first -- this was already set in my
> profile for
>       some reason. Not sure why.)
> 
>  3. Visit https://maps.google.com/?vector=1  (which should enable MapsGL)
> 
> ACTUAL RESULTS: Crash, with this printed to my terminal:
> firefox: nvc0_surface.c:218: nvc0_resource_copy_region: Assertion
> `src->nr_samples == dst->nr_samples' failed.
> 
> Sample crash reports:
>  bp-63b77e54-c5e4-45c6-b933-c26a32120730
>  bp-5ec26eac-8033-4661-977d-b52222120730

Here you say when it's when 'msaa-level=1', but the bug title says '=2'.
Is it both, or just one of them?

It looks like there could have been an issue with using msaa-level=1 that was fixed in:
http://cgit.freedesktop.org/mesa/mesa/commit/src/gallium/drivers/nvc0/nvc0_surface.c?id=b328949a37fee7b0f68ed3e068ffc4426c083042

Unfortunately, this isn't in 8.0.3, and it's also not in the 8.0.4 package available on their FTP.

Instead of blocklisting here, we should probably just round 'samples=1' down to 'samples=0' here. (We should probably be doing this everywhere, anyways)

If it's instead a problem with 'aa=2', then we should blocklist AA on this platform.
Comment 7 Jeff Gilbert [:jgilbert] 2012-07-30 13:14:00 PDT
Comment on attachment 647224 [details] [diff] [review]
block MSAA on nouveau

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

R+ if this is a 'samples=2' problem, but if it's a 'samples=1' problem, we should probably just work around this.
Comment 8 Daniel Holbert [:dholbert] 2012-07-30 19:57:26 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Here you say when it's when 'msaa-level=1', but the bug title says '=2'.
> Is it both, or just one of them?

It's msaa-level = 1.  Sorry for the typo on that.  (2 is currently the default, but my profile had 1 instead for some reason, and 1 is the value that causes crashes.)
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-07-31 18:23:37 PDT
Jeff, didn't we say at some point that we should silently interprete 1 as 0 ?

That would work around the problem without user-noticeable effects, right?
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-07-31 18:24:37 PDT
Oh right, I guess we wanted to keep 1 != 0 for testing/debugging purposes. But we should special-case Nouveau there. The GL_RENDERER string allows to identify Nouveau, see GfxInfoX11.cpp.
Comment 11 Jeff Gilbert [:jgilbert] 2012-08-01 15:18:21 PDT
(In reply to Benoit Jacob [:bjacob] from comment #10)
> Oh right, I guess we wanted to keep 1 != 0 for testing/debugging purposes.
> But we should special-case Nouveau there. The GL_RENDERER string allows to
> identify Nouveau, see GfxInfoX11.cpp.

I don't think it's worth keeping the '1xAA' stuff around, since it can be different from 'no AA' anyways. (sample locations, etc) We should dig up that old bug and finally force samples=1 to samples=0.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-08-02 13:49:41 PDT
Created attachment 648472 [details] [diff] [review]
samples: change 1 into 0

Like this? This doesn't catch direct calls to renderbufferstoragemultisample, but we don't have any at the moment.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-08-03 08:08:47 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/0d1da7c6afd5
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-08-03 08:11:17 PDT
Comment on attachment 648472 [details] [diff] [review]
samples: change 1 into 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing driver bug, that we run into at least since we turned on WebGL antialiasing in Firefox 10
User impact if declined: crash, possibly exploitable, in nouveau driver (concerns maybe 10%-20% of linux users, so maybe 0.1% of total users), triggerable by WebGL
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): No risk, trivial patch
String or UUID changes made by this patch: none
Comment 15 Ed Morley [:emorley] 2012-08-04 11:25:17 PDT
https://hg.mozilla.org/mozilla-central/rev/0d1da7c6afd5
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-06 12:10:18 PDT
Comment on attachment 648472 [details] [diff] [review]
samples: change 1 into 0

trivial patch, not sure if it's going to be sec:critical or not but willing to take on branches regardless at this stage in the cycle.
Comment 17 Daniel Veditz [:dveditz] 2012-08-09 16:40:58 PDT
Given it's a driver bug workaround I'm going to assume ESR is affected.

Rating this sec-moderate on the assumption--possibly mistaken--that the msaa setting is non-default and the average user is unlikely to change it. That may be incorrect if some popular software or addon fiddles this pref for users, though. Since the workaround is simple and safe we should perhaps take this anyway on ESR.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-09 16:42:36 PDT
please nominate for ESR uplift as well, assuming that this lands cleanly - let us know if that's not the case.
Comment 19 Daniel Holbert [:dholbert] 2012-08-09 16:50:08 PDT
(In reply to Daniel Veditz [:dveditz] from comment #17)
> Rating this sec-moderate on the assumption--possibly mistaken--that the msaa
> setting is non-default and the average user is unlikely to change it. That
> may be incorrect if some popular software or addon fiddles this pref for
> users, though.

The bad value (webgl.msaa-level = 1) is currently non-default, that's true.

However, I don't recall ever touching that pref manually, so I don't know how it got set to the non-default bad value in my profile.  It might've be from an intermediate nightly version temporarily setting it to 1, or perhaps from an addon setting it, as dveditz suggested.

I suppose it's also remotely possible that someone blogged and recommended setting it to 1 at some point in the past, to test a piece of New Shiny, and I toggled it then and forgot about it... Perhaps bjacob/jgilbert know whether it's possible that any Planet blogs might've suggested toggling it in the past?

(Just discussing this in the interest of guessing how many other people might have it set to 1, for whatever reason.)
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 19:26:53 PDT
webgl.msaa-level=1 is non-default indeed, BUT this is not the only way to reproduce this. If the driver reports that 1 is the max supported value, we'll adjust down to it, to avoid exceeding it. That's why this bug could be reproduced in default configuration.

The patch probably won't apply cleanly to ESR, but making a ESR patch shouldn't be hard.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-08-10 14:33:10 PDT
For Firefox 10 ESR, notice that in bug 777028 we are blacklisting all Mesa drivers for WebGL in ESR10. This will, in particular, fix this issue.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-08-15 17:29:29 PDT
Still pending Firefox ESR 10 patch from bug 777028.
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-08-22 14:49:31 PDT
ESR10 patch in bug 777028 just landed.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 11:01:15 PDT
I'm unable to reproduce this with the 2012-07-19 Firefox 17.0a1 Debug build on Ubuntu 12.04 64-bit. I followed the steps in comment 0 and Google MapsGL works without crashing. I checked my graphics driver and it's reported as "2.1 Mesa 8.0.2" in about:support.

Does this require a specific graphic chipset/driver?
Comment 26 Daniel Holbert [:dholbert] 2012-08-24 11:12:24 PDT
I verified on my system that an old nightly (from the comment 0 date) still crashes with STR:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/07/2012-07-30-03-05-40-mozilla-central/firefox-17.0a1.en-US.linux-x86_64.tar.bz2
...and that today's nightly does not crash:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/08/2012-08-24-03-05-30-mozilla-central/firefox-17.0a1.en-US.linux-x86_64.tar.bz2

FWIW, I'm using Ubuntu 12.10 alpha (up-to-date), with about:support saying:
  Driver Version:  3.0 Mesa 8.0.4
  WebGL Renderer:  nouveau -- Gallium 0.4 on NVC1 -- 3.0 Mesa 8.0.4

Marking as VERIFIED.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 11:17:45 PDT
That same Nightly does not crash for me so I guess this is driver dependent. Daniel, if you have time, can you please test if this is fixed with the latest Firefox 15, 10.0.7esr, and 16.0a2 (in order of priority)?

Thank you.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-08-24 12:03:45 PDT
Yes, this is specific to the Nouveau driver.

Note You need to log in before you can comment on or make changes to this bug.