1072501 - We probably don't use DIBTextureClient with e10s

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I expect this will break subpixel AA

Comment 1

[Jeff Muizelaar [:jrmuizel]]

subpixel when not using direct2d

Comment 2

[Jim Mathies [:jimm]]

Jeff, is this a serious e10s related bug? Nobody is sure what this is about.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

We create a DIBTexture client here only when in the same process:
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#296

This is needed for proper subpixel-aa text and maybe performance.

This can probably be fixed by creating a DIBSection around shared memory like we do for plugins.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

It would help us if E10S team can figure out which of your milestones needs this so that we can schedule it correctly.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> It would help us if E10S team can figure out which of your milestones needs
> this so that we can schedule it correctly.

It's up to the graphics team, can we live without whatever this is? In triage yesterday we just gave it a plus, implying this is considered post-enable on nightly cleanup fodder.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks, that helps, wasn't sure what that plus meant.

Comment 7

[cmtalbert]

Hey Jeff,

Can you help me understand if I'm reading this bit of code correctly?  It looks like we might have E10S issues on Windows XP sytems where we are using either total software rendering with no direct x support at all, and rendering something with BGR color formats, which is the line of code you reference, and that we might have issues with D3D9 on XP on E10S (the previous if statement right above the one you reference).  I see other places where aAllocator->IsSameProcess is used in this method, but the usage in the XP ifdef concerns me most because trying to get real-world preliminary testing in that area is going to be tough.

Also, what is a real world case where we would use BGR color space? I can't exactly wrap my head around when we tend to encounter surfaces of that type.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Clint Talbert ( :ctalbert ) from comment #7)
> Hey Jeff,
> 
> Can you help me understand if I'm reading this bit of code correctly?  It
> looks like we might have E10S issues on Windows XP sytems where we are using
> either total software rendering with no direct x support at all, and
> rendering something with BGR color formats, which is the line of code you
> reference, and that we might have issues with D3D9 on XP on E10S (the
> previous if statement right above the one you reference).  I see other
> places where aAllocator->IsSameProcess is used in this method, but the usage
> in the XP ifdef concerns me most because trying to get real-world
> preliminary testing in that area is going to be tough.
> 
> Also, what is a real world case where we would use BGR color space? I can't
> exactly wrap my head around when we tend to encounter surfaces of that type.

This is unrelated to Windows XP. The problems should show up anytime we're not using Direct2D. I'm not sure the other cases matter.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Has anything changed on this front, as in what we think we need to do and what we have done?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #9)
> Has anything changed on this front, as in what we think we need to do and
> what we have done?

No, the basic fix for this is 'in the tree', but when we enable it we see mysterious crashes occur in the wild inside cairo. I have not heard of any way to reproduce them, we haven't attempted to re-enable it and figure out more data from crash reports - we could.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Is it expected that end up in the "UNSUPPORTED" section of cairo_win32_surface_composite?

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Is it expected that end up in the "UNSUPPORTED" section of
> cairo_win32_surface_composite?

Hmm, probably is, although I still don't like the "unsupported" part.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Read access violation on the surface in the pattern argument to DrawTargetCairo::DrawPattern.  Not null, but "bad".  Don't know if it's a UAF or some uninitialized value.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Crash stack for the "click on a crash in about:crashes" workflow

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Manage the mapped data

(In reply to Milan Sreckovic [:milan] from comment #13)
> Read access violation on the surface in the pattern argument to
> DrawTargetCairo::DrawPattern.  Not null, but "bad".  Don't know if it's a
> UAF or some uninitialized value.

Neither - we use the memory after we've unmapped it; it'd be surprising if that worked :) TextureHostFileMapping::UpdatedInternal unmaps it, after giving it to texture source to hold, and eventually passing it outside through BindTextureSource.  I don't know what the design had in mind here - for us to keep the data mapped inside of TextureHostFileMapping or to make a copy in the bind method, or something else?

Anyway, something like this works, but I don't know if the data that gets outside of this class lives after the class goes away.

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8691064 [details] [diff] [review]
Manage the mapped data

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

Hrm, so this is interesting. I wrote this code, the intention was to minimize address space usage by unmapping the file when we no longer need it (i.e. when we're done with the surface), if the texture source apparently can grab a reference internally, I believe we should store userdata on the surface and have that unmap the file when the surface is destroyed. This would maintain the minimizing address space usage in the parent process but at the same time keep the memory around for as long as the DataSourceSurface is. This patch would obviously do the trick, but f- since I'd like to know if that approach gets us the best of both worlds.

Nice catch.

Comment 18

[Bas Schouten (:bas.schouten)]

Attachment: Unmap file mapping on source surface destruction.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8691533 [details] [diff] [review]
Unmap file mapping on source surface destruction.

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

This fixes the crash I was seeing (with suitable rebasing to central.)  Offline conversation established that we won't have the same map given to multiple surfaces.

I am assuming that the TextureHostFileMapping will not outlive the surface that uses the mapped data, including during the shutdown?

::: gfx/layers/TextureDIB.cpp
@@ +236,2 @@
>  
>    ::UnmapViewOfFile(fileMapping);

I know this isn't part of the change, but shouldn't this be ::UnmapViewOfFile(data) instead?

Comment 20

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #19)
> Comment on attachment 8691533 [details] [diff] [review]
> Unmap file mapping on source surface destruction.
> 
> Review of attachment 8691533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This fixes the crash I was seeing (with suitable rebasing to central.) 
> Offline conversation established that we won't have the same map given to
> multiple surfaces.
> 
> I am assuming that the TextureHostFileMapping will not outlive the surface
> that uses the mapped data, including during the shutdown?

If it would we'd see issues in other texture clients I believe. I'm also not sure if you close the handle but not unmap the memory actually goes away but it shouldn't be relevant.

> ::: gfx/layers/TextureDIB.cpp
> @@ +236,2 @@
> >  
> >    ::UnmapViewOfFile(fileMapping);
> 
> I know this isn't part of the change, but shouldn't this be
> ::UnmapViewOfFile(data) instead?

Very good catch. Absolutely true. Fixed locally, I will land this patch after Nical lands his queue, I don't want to bitrot him.

Comment 21

[Pulsebot]

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

Comment 22

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

I had to back this out for various t-e10s failures on Windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c813f2aeb7a

https://treeherder.mozilla.org/logviewer.html#?job_id=18093143&repo=mozilla-inbound

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

as a note, this caused a 10% win for tps, the backout saw a regression- looking forward to this landing again!

Comment 24

[Bas Schouten (:bas.schouten)]

(In reply to Joel Maher (:jmaher) from comment #23)
> as a note, this caused a 10% win for tps, the backout saw a regression-
> looking forward to this landing again!

I honestly don't see any way whatsoever that this test failure would be related to my patch so it might be a little tricky :).

Comment 25

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #24)
> (In reply to Joel Maher (:jmaher) from comment #23)
> > as a note, this caused a 10% win for tps, the backout saw a regression-
> > looking forward to this landing again!
> 
> I honestly don't see any way whatsoever that this test failure would be
> related to my patch so it might be a little tricky :).

I was reading the log poorly, I think I understand the problem better now.

Comment 26

[Pulsebot]

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

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=18156523&repo=mozilla-inbound - it was a little hard to spot since you landed on top of other gfx talos e10s bustage, but https://treeherder.mozilla.org/logviewer.html#?job_id=18160878&repo=mozilla-inbound#L6491 is a pretty good example.

Comment 28

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d49a71d3b20a

Comment 29

[Phil Ringnalda (:philor)]

Yeah, that was what I meant to say.

Comment 30

[Bas Schouten (:bas.schouten)]

Attachment: Unmap file mapping on source surface destruction and make UpdateFromSurface work for ShmemDIBTextureData.

This fixes the issue that was causing the reftest failure.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/975af6bc574f

Comment 32

[Phil Ringnalda (:philor)]

That's slightly better, in that it fooled me into thinking it was just normal talos-e10s bustage for a little while, but it's still busted. Tree's closed.

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d11cceb2f732 - Win32-only, t-e10s-only, g1 and s and tp, perhaps https://treeherder.mozilla.org/logviewer.html#?job_id=18418348&repo=mozilla-inbound#L2445 is a clue as to what's failing before the unhelpful "TalosError: timeout".

Comment 34

[Phil Ringnalda (:philor)]

Also busted in Win32 Marionette, with one of two crashes, https://treeherder.mozilla.org/logviewer.html#?job_id=18429747&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=18429748&repo=mozilla-inbound

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, next time we land this, what do you think about having a preference that lets us "turn it off" - we'd still have it be on by default, but I'd like the ability to turn it off in case we need to investigate possible fallback.  (gfx.layers.dib.disable or some such, false by default.)

Comment 36

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #35)
> Bas, next time we land this, what do you think about having a preference
> that lets us "turn it off" - we'd still have it be on by default, but I'd
> like the ability to turn it off in case we need to investigate possible
> fallback.  (gfx.layers.dib.disable or some such, false by default.)

Yeah, once I get it to pass the tests I'll add that.

Comment 37

[Pulsebot]

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

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Milan, should we uplift this to 45 for our e10s beta experiment?

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

I would prefer to let it ride the trains; I'm not convinced all that we need is contained in this bug, and this is a minor visual glitch that I can't see really screwing up the e10s beta experiment.