Closed Bug 880734 Opened 11 years ago Closed 11 years ago

New Google Maps does not show/update the map when being loaded or dragged around

Categories

(Core :: Graphics: CanvasWebGL, defect)

23 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 + wontfix
firefox23 + verified
firefox24 + verified
firefox-esr17 23+ fixed
b2g18 --- unaffected

People

(Reporter: whimboo, Assigned: jgilbert)

References

()

Details

(Keywords: crash, sec-critical, sec-vector, Whiteboard: [adv-main23+][adv-esr1708+])

Attachments

(4 files, 1 obsolete file)

Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130607 Firefox/24.0 ID:20130607031055 CSet: dc8e78ed8c44

The new Google Maps does not show the map during initial load and does even not get updated when you drag the map around. It stays empty all the time. It works fine in Epiphany on Linux, and also on OS X. Not sure if that is a hardware configuration or not. After turning off hardware acceleration also doesn't help. It stays blank. As I noticed there are situation when the map actually gets shown but I wasn't able to reproduce it yet.

I'm working with a Lenovo X230 on Ubuntu 13.04. Also keep in mind that you need an invitation to use the new Google maps.

Not sure how many people will be affected by this yet, but leaving this up for the drivers to decide how important it is.
Google reported some issues to us. IIRC roc was looking at those.
Btw. I don't see any error listed in the error console.
Can qa look at getting an account/invite to the new google maps so there can be testing/verification around this?

Roc - are you already looking into this in any other bug(s)?

I'm not too concerned about this for FF22 since it's still invite-only but we should try to get a jump on this given we don't control when they'll make it the default.
Flags: needinfo?(roc)
Keywords: qawanted
It works fine for me on my Linux system. We need to figure out what the common factor is for the users who see this bug. I suspect something GL-related. I'm using the latest Nvidia GL driver. Henrik, what about you?
Flags: needinfo?(roc)
Is the graphics information from about:support enough? If not please tell me where to find the necessary information.

> Adapter Description	Intel Open Source Technology Center -- Mesa DRI Intel(R) Ivybridge Mobile
> Device ID	Mesa DRI Intel(R) Ivybridge Mobile
> Driver Version	3.0 Mesa 9.1.1
> GPU Accelerated Windows	0/2 Basic
> Vendor ID	Intel Open Source Technology Center
> WebGL Renderer	Intel Open Source Technology Center -- Mesa DRI Intel(R) Ivybridge Mobile
> AzureCanvasBackend	cairo
> AzureContentBackend	none
> AzureFallbackCanvasBackend	none
From bug 877330:

Martin 'Nexus' Filip (New to Bugzilla) 2013-06-08 07:21:30 PDT

I think the problem is a combination of Firefox + WebGL + Linux + Intel graphics card.
I'm using Arch Linux, same version of Firefox (22.0b4-1) on two machines. One with nvidia proprietaly driver works fine, one with intel card does not work and produces same symptoms as described in this and some other tickets (#873414, #875278, #880734).

Not working one:
Graphics
Adapter Description Intel Open Source Technology Center -- Mesa DRI Intel(R) Ivybridge Mobile
Device ID Mesa DRI Intel(R) Ivybridge Mobile
Driver Version 3.0 Mesa 9.1.3
GPU Accelerated Windows 0/1 Basic
Vendor ID Intel Open Source Technology Center
WebGL Renderer Intel Open Source Technology Center -- Mesa DRI Intel(R) Ivybridge Mobile
AzureCanvasBackend cairo
AzureContentBackend none
AzureFallbackCanvasBackend none
This is probably a WebGL or layers bug.
Component: DOM: Core & HTML → Canvas: WebGL
Karl, can you reproduce this bug?
Assignee: nobody → karlt
It's a Mesa bug of some sort -- Google Maps renders fine in Firefox when using the AMD Catalyst drivers.

On the other hand, it renders fine in Chrome using Mesa, so it may be something that is worth working around.
A GL_INVALID_FRAMEBUFFER_OPERATION error is generated in
glClear(GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT|GL_STENCIL_BUFFER_BIT).
glCheckFramebufferStatus returns GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE.

http://www.opengl.org/sdk/docs/man3/xhtml/glCheckFramebufferStatus.xml:

"GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE is returned if the value of
 GL_RENDERBUFFER_SAMPLES is not the same for all attached renderbuffers; if
 the value of GL_TEXTURE_SAMPLES is the not same for all attached textures;
 or, if the attached images are a mix of renderbuffers and textures, the value
 of GL_RENDERBUFFER_SAMPLES does not match the value of GL_TEXTURE_SAMPLES.

 GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE is also returned if the value of
 GL_TEXTURE_FIXED_SAMPLE_LOCATIONS is not the same for all attached textures;
 or, if the attached images are a mix of renderbuffers and textures, the value
 of GL_TEXTURE_FIXED_SAMPLE_LOCATIONS is not GL_TRUE for all attached
 textures."
                                     
Running with MESA_EXTENSION_OVERRIDE=-GL_EXT_framebuffer_multisample in the
environment works around the bug.
Quoting Grigori Goronzy at https://bugs.freedesktop.org/show_bug.cgi?id=65514#c4

"To be more accurate, Google Maps attaches a depth and stencil buffer with 4 samples and a colorbuffer with 0 (1) samples. That's clearly not supposed to work. I do not believe this is a bug in Mesa."

I assume it is the browser's job to ensure this doesn't happen.
Jeff: opinions on comments 13 -- 14 ?
Flags: needinfo?(jgilbert)
I think I'm seeing a draw framebuffer that contains multisample color, depth,
and stencil renderbuffer attachments.  Then glFramebufferTexture2D with GL_TEXTURE_2D replaces the color buffer, something tries to disable the depth buffer through GL_DEPTH_TEST (stencil already disabled), and the webgl clear fails.

apitrace file (1.6 MB) at http://people.mozilla.org/~karlt/880734.trace

OpenGL version is "3.0 Mesa 9.0.3".

http://www.opengl.org/sdk/docs/man3/xhtml/glBindTexture.xml says
"The GL_TEXTURE_2D_MULTISAMPLE and GL_TEXTURE_2D_MULTISAMPLE_ARRAY targets are available only if the GL version is 3.2 or higher."

ARB_texture_multisample is not advertised.

I don't know whether the browser can separate the use of texture and
renderbuffer attachments, or whether that is the responsibility of the WebGL
client, or whether multisample renderbuffer attachments should not be used
when GL_TEXTURE_2D_MULTISAMPLE is not available.
We'll track for FF23 and on (since that's the timeframe that the new Google Maps may come out of Beta), but it's very unlikely that a fix will land in the FF22 timeframe.
(In reply to Karl Tomlinson (:karlt) from comment #14)
> Quoting Grigori Goronzy at
> https://bugs.freedesktop.org/show_bug.cgi?id=65514#c4
> 
> "To be more accurate, Google Maps attaches a depth and stencil buffer with 4
> samples and a colorbuffer with 0 (1) samples. That's clearly not supposed to
> work. I do not believe this is a bug in Mesa."
> 
> I assume it is the browser's job to ensure this doesn't happen.

Yes, this sounds a lot like our fault.
Flags: needinfo?(jgilbert)
(In reply to Karl Tomlinson (:karlt) from comment #16)
> I think I'm seeing a draw framebuffer that contains multisample color, depth,
> and stencil renderbuffer attachments.  Then glFramebufferTexture2D with
> GL_TEXTURE_2D replaces the color buffer, something tries to disable the
> depth buffer through GL_DEPTH_TEST (stencil already disabled), and the webgl
> clear fails.
> 
> apitrace file (1.6 MB) at http://people.mozilla.org/~karlt/880734.trace
> 
> OpenGL version is "3.0 Mesa 9.0.3".
> 
> http://www.opengl.org/sdk/docs/man3/xhtml/glBindTexture.xml says
> "The GL_TEXTURE_2D_MULTISAMPLE and GL_TEXTURE_2D_MULTISAMPLE_ARRAY targets
> are available only if the GL version is 3.2 or higher."
> 
> ARB_texture_multisample is not advertised.
> 
> I don't know whether the browser can separate the use of texture and
> renderbuffer attachments, or whether that is the responsibility of the WebGL
> client, or whether multisample renderbuffer attachments should not be used
> when GL_TEXTURE_2D_MULTISAMPLE is not available.

ARB_texture_multisample is not something we use, since we only use MSRBs. It sounds like our logic for this is messing up somewhere. I'll check.
Group: core-security
Our previous fix for bug 814407 was incomplete. It didn't actually bind to the correct framebuffer before trying to attach the texture to it.

We should assert our assumptions in code that relies on the currently bound state of GL.
Assignee: karlt → jgilbert
Attachment #761228 - Flags: review?(bjacob)
Requesting re-evaluation of tracking for Beta 22 in light of security concerns.
Oops, previous patch had leakage from another patch.
Attachment #761228 - Attachment is obsolete: true
Attachment #761228 - Flags: review?(bjacob)
Attachment #761255 - Flags: review?(bjacob)
Comment on attachment 761255 [details] [diff] [review]
patch: Assert that we're the current FB in FB funcs, and bind the correct FB during Mesa-only tex-rebind-after-update driver workaround

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +5446,5 @@
>          framebuffer;
>          framebuffer = framebuffer->getNext())
>      {
>          if (framebuffer->ColorAttachment().Texture() == tex) {
> +            ScopedBindFramebuffer autoFB(gl, framebuffer->GLName());

Hah. My bad, sorry. Also goes to explain how that bug would show only on Mesa, as IIRC we only perform this work-around on Mesa.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +152,5 @@
>                               WebGLenum attachment,
>                               WebGLenum rbtarget,
>                               WebGLRenderbuffer *wrb)
>  {
> +    MOZ_ASSERT(mContext->mBoundFramebuffer == this);

Great assertions to have.
Attachment #761255 - Flags: review?(bjacob) → review+
(In reply to Alex Keybl [:akeybl] from comment #17)
> We'll track for FF23 and on (since that's the timeframe that the new
> Google Maps may come out of Beta)

Re-nominating for Fx22 tracking since this is a WebGL security bug that other sites could trigger (in users with Mesa drivers). Then again, Mesa is not a huge part of our market. Patch looks pretty trivially safe to me but I'll defer to Jeff and Benoit on that if they think not.
(In reply to Daniel Veditz [:dveditz] from comment #25)
> Re-nominating for Fx22 tracking since this is a WebGL security bug that
> other sites could trigger (in users with Mesa drivers). Then again, Mesa is
> not a huge part of our market. Patch looks pretty trivially safe to me but
> I'll defer to Jeff and Benoit on that if they think not.

Given this risk/reward, likely a wontfix for FF22 still at this point.
Comment on attachment 761255 [details] [diff] [review]
patch: Assert that we're the current FB in FB funcs, and bind the correct FB during Mesa-only tex-rebind-after-update driver workaround

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Non-trivial for the complete exploit, but it's clear how to hit bug 814407's use-after-free from this patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, but secbug-gone-public 814407 is the only blame in this part of the code.

Which older supported branches are affected by this flaw?
Everything 17+, including ESR17.

If not all supported branches, which bug introduced the flaw?
Bug 814407 incompletely fixed this, but was made public.

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

How likely is this patch to cause regressions; how much testing does it need?
It needs a test, especially since we failed to completely fix this before.
Attachment #761255 - Flags: sec-approval?
Comment on attachment 761255 [details] [diff] [review]
patch: Assert that we're the current FB in FB funcs, and bind the correct FB during Mesa-only tex-rebind-after-update driver workaround

sec-approval+ for trunk. Please get this in and make and nominate an aurora path for Firefox 23.
Attachment #761255 - Flags: sec-approval? → sec-approval+
I think the checkin-needed keyword is the RyanVM batsignal.
Attachment #761255 - Flags: checkin? → checkin+
Looks like caching reset the tracking flag. :\
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0453f3ed70

Mac compiler differences are driving me up the wall.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 814407
User impact if declined: New Google Maps won't work for Linux+IntelGPU users, as well as the outstanding security concern from the regressing bug.
Testing completed (on m-c, etc.): None
Risk to taking this patch (and alternatives if risky): Very low. Fix is very straightforward.
String or IDL/UUID changes made by this patch: None
Attachment #766155 - Flags: review+
Attachment #766155 - Flags: approval-mozilla-aurora?
Attachment #766159 - Attachment description: patch for landing after exn- → patch for landing after ext-draw-buffers
Attachment #766159 - Flags: review+
Hmm, the checkin-needed flag here confused me.  I saw there are some hunks which have already been landed by another patch, and didn't check the contents of the commit before pushing it, so ended up pushing an empty patch :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee0caa5506d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f0aee7807f9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Works great now with Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130623 Firefox/24.0 ID:20130623031022 CSet: 4c4f75c20e9b. Thanks!
Status: RESOLVED → VERIFIED
Attachment #766155 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 766155 [details] [diff] [review]
patch for landing

[Approval Request Comment]
Missed the Aurora train. Needs beta approval.
Attachment #766155 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta?
Comment on attachment 766155 [details] [diff] [review]
patch for landing

sorry about that, pre-merge approval request.
Attachment #766155 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified fixed with latest beta: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 ID:20130708202947 CSet: d318fea569d6
Comment on attachment 766155 [details] [diff] [review]
patch for landing

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-crit
Fix Landed on Version: 24,23
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #766155 - Flags: approval-mozilla-esr17?
Attachment #766155 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
https://hg.mozilla.org/releases/mozilla-esr17/rev/19a4edced33b

This required a little fixing up for esr17. Please take a look to make sure I didn't mess it up (assuming it sticks).
Funny enough, it was one of the hunks that *did* apply cleanly that broke things. Irony++

Due to esr17 lacking in bug 716859 and by extension ScopedBindFramebuffer, this didn't build. Backed out.
https://hg.mozilla.org/releases/mozilla-esr17/rev/d755488e2560

https://tbpl.mozilla.org/php/getParsedLog.php?id=25662458&tree=Mozilla-Esr17
Flags: needinfo?(jgilbert)
Thanks for trying anyways. I can fix it up and land it tomorrow morning.
Flags: needinfo?(jgilbert)
Whiteboard: [adv-main23+]
De-bit-rotted.
This applies cleanly, and should build and everything.
If it doesn't, shame on me.
Attachment #782081 - Flags: review+
Attachment #782081 - Flags: checkin?
Attachment #782081 - Attachment is patch: true
Attachment #782081 - Attachment mime type: message/rfc822 → text/plain
Whiteboard: [adv-main23+] → [adv-main23+][adv-esr1708+]
Henrik, when you are back from your PTO, it would be great if you could check that this is fixed for ESR17 on your machine. I don't have the hardware required to do so. Thank you.
(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> patch for landing after ext-draw-buffers

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #36)
> I saw there are some hunks
> which have already been landed by another patch,

On m-c, this bug was actually fixed by https://hg.mozilla.org/mozilla-central/rev/e13b9e00a78d in bug 843667.

(In reply to Phil Ringnalda (:philor, back Aug 5) from comment #37)
> https://hg.mozilla.org/mozilla-central/rev/8f0aee7807f9

So this change just added the assertion.
Depends on: 901297
Group: core-security
You need to log in before you can comment on or make changes to this bug.