Alpha extraction in plugin subprocesses is broken on windows

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][has patch][needs to land with 626602])

Attachments

(5 attachments, 2 obsolete attachments)

The code is very broken, because we try to paint onto an image surface, but that requires a dc.  This is a s-s because we end up making invalid static casts to SharedDIBSurface* from vanilla image surface, then passing the HDC to the plugin, e.g. here

2525     if (curSurface) {
2526         NS_ASSERTION(SharedDIBSurface::IsSharedDIBSurface(curSurface),
2527                      "Expected (SharedDIB) image surface.");
2528 
2529         SharedDIBSurface* dibsurf = static_cast<SharedDIBSurface*>(curSurface.get());
2530         dc = dibsurf->GetHDC();

I don't know if this is exploitable, just crashy, or windows sanitizes these, but better safe than sorry etc.
This needs to block for being broken and possibly exploitable or crashy.
blocking2.0: --- → ?
Not s-s, this code can't be hit in the wild.
Group: core-security
blocking2.0: ? → ---
I'm not quite sure under what conditions we would enable alpha extraction on windows.  If drawing-on-background fails, in which cases we would try to use the current set of hacks and in which cases would we fall back on alpha extraction?  Is the idea to remove the current hacks and always fall back on alpha extraction, since we're betting on it being an uncommon case?
Created attachment 508050 [details] [diff] [review]
Fix alpha recovery fallback on windows

Thanks Jim, I cannibalized your patch from bug 611698.

I'm not sure whether this bug should block bug 626602 or not, and whether there's code we want to get rid of in favor of the normal path in 626602 and the fallback here.
Assignee: nobody → jones.chris.g
Attachment #508050 - Flags: review?(jmathies)
We definitely need this. The "current set of hacks" --- i.e., painting the plugin to a DIB and hoping we get the right alpha values --- fails in unpredictable ways, so we just can't do that.
Comment on attachment 508050 [details] [diff] [review]
Fix alpha recovery fallback on windows

I should probably review this since jimm wrote part of it...

+    // Paint the plugin directly onto the target, with a white
+    // background and copy the result
+    PaintRectToSurface(aRect, aSurface, gfxRGBA(1.0, 1.0, 1.0));
+    {
+        gfxRect copyRect(gfxPoint(0, 0), targetRect.size);
+        nsRefPtr<gfxContext> ctx = new gfxContext(whiteImage);
+        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
+        ctx->SetSource(aSurface, deviceOffset);
+        ctx->Rectangle(copyRect);
+        ctx->Fill();

Isn't this copy avoidable? Can't whiteImage be a temporary DIB surface, that we draw into and then pass a gfxImageSurface wrapper around the same bits to RecoverAlpha?
Attachment #508050 - Flags: review?(jmathies) → review?(roc)
Also, while we're changing the temp surface allocation, we should do something to ensure that the buffers have the same alignment relative to 16-byte boundaries, to ensure the SSE2 RecoverAlpha code is used.
(In reply to comment #7)
> Isn't this copy avoidable? Can't whiteImage be a temporary DIB surface, that we
> draw into and then pass a gfxImageSurface wrapper around the same bits to
> RecoverAlpha?

Yes.

(In reply to comment #8)
> Also, while we're changing the temp surface allocation, we should do something
> to ensure that the buffers have the same alignment relative to 16-byte
> boundaries, to ensure the SSE2 RecoverAlpha code is used.

That's kinda tricky unless we always want to create a temporary blackImage.
I don't see why. Actually, in my testing a SharedDIBSurface's bits are always aligned to a page or something like that. So all we need to do is ensure that the temporary whiteImage bits are allocated at an aligned offset.
The strides need to match up properly as well.  Maybe this is easier than I think, I'll take a look after finishing some other stuff.
Jim, what all needs to be set up for the plugin to paint to a temporary gfxWindowsSurface smaller than its frame?  I tried setting this up by adding a gfxASurface param to UpdateWindowAttributes() and using its DC if non-null, but I got messed-up painting from flash with that, I suspect because other window attributes didn't match up with the DC.

In the meantime, I'll try to get a new patch up that aligns things properly for SSE2 alpha recovery.

Comment 13

8 years ago
(In reply to comment #12)
> Jim, what all needs to be set up for the plugin to paint to a temporary
> gfxWindowsSurface smaller than its frame?  I tried setting this up by adding a
> gfxASurface param to UpdateWindowAttributes() and using its DC if non-null, but
> I got messed-up painting from flash with that, I suspect because other window
> attributes didn't match up with the DC.
> 
> In the meantime, I'll try to get a new patch up that aligns things properly for
> SSE2 alpha recovery.

Plugins require a number of setup calls if you're switching out surfaces, all of which happen in UpdateWindowAttributes and PaintRectToPlatformSurface. All of these rely on data in mWindow, is that getting updated correctly?

Comment 14

8 years ago
I'd suggest overhauling the code in those two routines so they don't use mWindow at all. Everything should get passed in so we have better control over what gets rendered to. We only call UpdateWindowAttributes from two places, DoAsyncSetWindow and PaintRectToPlatformSurface.
I looked a bit more last night and I'm not sure how using a temporary surface is going to work.  It seems like with the code that's there, I would need to change the window size, but that seems wrong (don't want flash "reflowing").  And experience shows flash doesn't deal with a DC for the smaller surface.

The X code does the same thing too, only hands out surfaces that are the size of the frame.  I think this might just be a limitation we'll have to live with for the time being, unless I'm missing something.
Created attachment 508194 [details] [diff] [review]
part 1: Add some helpers for aligning surfaces for alpha recovery

I have the nagging feeling that I'm missing a cleverer solution for the rect-nudging algorithm, but what's there should be OK.
Attachment #508050 - Attachment is obsolete: true
Attachment #508194 - Flags: superreview?(roc)
Attachment #508050 - Flags: review?(roc)
Created attachment 508195 [details] [diff] [review]
part 2: Ensure that gfxImageSurfaces are aligned well for alpha recovery
Attachment #508195 - Flags: review?(roc)
Created attachment 508196 [details] [diff] [review]
part 3: Ensure that SharedDIBWins are aligned well for alpha recovery
Attachment #508196 - Flags: review?(jmathies)
Created attachment 508197 [details] [diff] [review]
Fix alpha-recovery fallback on windows and try to use the SIMD path when possible

This uses the copy-back-to-temporary approach because I don't know how to give flash a temporary windows surface.

In totally unscientific tests of the GUIMark wmode=transparent benchmark from bug 626602, I saw fps go from ~13fps to ~15fps.
Attachment #508197 - Flags: review?(roc)
Created attachment 508200 [details] [diff] [review]
part 1: Add some helpers for aligning surfaces for alpha recovery, v2

Crap, I realized that a simplification I made to this earlier today was wrong.  Fixed.
Attachment #508194 - Attachment is obsolete: true
Attachment #508200 - Flags: superreview?(roc)
Attachment #508194 - Flags: superreview?(roc)
(In reply to comment #14)
> I'd suggest overhauling the code in those two routines so they don't use
> mWindow at all. Everything should get passed in so we have better control over
> what gets rendered to. We only call UpdateWindowAttributes from two places,
> DoAsyncSetWindow and PaintRectToPlatformSurface.

FWIW, I totally agree with this, but I think we should wholesale rewrite PluginInstanceChild/Parent.  The current code is a disgusting rat's nest.  I have some ideas for how we can use the remote layers system to clean things up and make updates more efficient / less glitchy too.  Post-4.0 though! :)

(Although if we want async rendering on mac for 4.0, we may need this.)
Comment on attachment 508200 [details] [diff] [review]
part 1: Add some helpers for aligning surfaces for alpha recovery, v2

+    static PRUint32 GoodAlignmentlg2() { return 4; /* for SSE2 */ }

GoodAlignmentLog2

+    return (aX + aStride * aY) & ((1 << aTolg2) - 1);

"Tolg" is not a word :-)

+    // Thebes only supports ARGB32 CONTENT_COLOR_ALPHA surfaces, so
+    // assume we have one here.

Add an assertion on aSurface->GetFormat().
Attachment #508200 - Flags: superreview?(roc) → superreview+
(In reply to comment #22)
> Comment on attachment 508200 [details] [diff] [review]
> part 1: Add some helpers for aligning surfaces for alpha recovery, v2
> 
> +    static PRUint32 GoodAlignmentlg2() { return 4; /* for SSE2 */ }
> 
> GoodAlignmentLog2

Oops, fixed.  I'm dumb.

> +    return (aX + aStride * aY) & ((1 << aTolg2) - 1);
> 
> "Tolg" is not a word :-)
> 

aAlignToLog2?

> +    // Thebes only supports ARGB32 CONTENT_COLOR_ALPHA surfaces, so
> +    // assume we have one here.
> 
> Add an assertion on aSurface->GetFormat().

Done.
Whiteboard: [hardblocker]

Updated

8 years ago
Whiteboard: [hardblocker] → [hardblocker][has patch]

Updated

8 years ago
Attachment #508196 - Flags: review?(jmathies) → review+
blocking2.0: final+ → betaN+
backed out due to permaorange in linux reftests
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297267690.1297268369.1934.gz
reftest/tests/modules/plugin/test/reftest/plugin-alpha-zindex.html | image comparison (==)

http://hg.mozilla.org/mozilla-central/rev/548c6fb45f53
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
FTR, this failure is due to a pre-existing bug that was incidentally fixed in part 9 of bug 626602.  But we'll re-land all this stuff together so it doesn't really matter.
Created attachment 511533 [details] [diff] [review]
part 1.1: Fix thinko

The algorithm keeps the bottommost pixel fixed always, and keeps the rightmost fixed wrt finding a new x coordinate, but needs to check that the original rightmost pixel + dw is within the surface.  I was confusing right/top/w/h in my head.

(Ignore the *NULL = 5, just for debugging.)
Attachment #511533 - Flags: review?(roc)
Comment on attachment 511533 [details] [diff] [review]
part 1.1: Fix thinko

+            for (dw = 0; (dw < pixPerAlign) && (r + dw <= sw); ++dw) {

Shouldn't this actually compare "(r + dw)*bpp <= surfaceSize.stride"? I guess this is conservative.

I think this would be easier to understand if you named dw 'dr' instead and added some comments explaining that the d* values mean.
Attachment #511533 - Flags: review?(roc) → review+
(In reply to comment #29)
> Comment on attachment 511533 [details] [diff] [review]
> part 1.1: Fix thinko
> 
> +            for (dw = 0; (dw < pixPerAlign) && (r + dw <= sw); ++dw) {
> 
> Shouldn't this actually compare "(r + dw)*bpp <= surfaceSize.stride"? I guess
> this is conservative.

We already bail early if |bpp * surfaceSize.width != aSurface->Stride()|, so we know that |r + dw <= sw => (r + dw)*bpp <= surfaceSize.stride".

> I think this would be easier to understand if you named dw 'dr' instead and
> added some comments explaining that the d* values mean.

Yes, good ideas.
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs to land with 626602]

Updated

7 years ago
Depends on: 637181
You need to log in before you can comment on or make changes to this bug.