Closed Bug 777946 Opened 7 years ago Closed 7 years ago

Zooming windowless plugin frame can cause it to "bleed" outside bounds

Categories

(Core :: Plug-ins, defect)

13 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: karlt, Assigned: karlt)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #635405 +++

STR
 (1) Load URL above
 (2) Ctrl-+ page to zoom in a ways
 (3) Ctrl-0

The plugin ImageLayer ends up drawing outside its bounds and splatting content all over its surrounding ThebesLayer.  Nothing knows to invalidate what was splatted, so it can persist "forever".

Looks like a regression from 
http://hg.mozilla.org/mozilla-central/rev/1a345b043b47
which undid the fix in bug 635405.
FWIW, I see this with X11 basic layers but not GL layers.
I can reproduce in Firefox13.0.1 and Firefox14.0.1 with HWA off.
however, I cannot reproduce in Firefox15.0b2 and later.

Progression window(m-c)
Bad:
http://hg.mozilla.org/mozilla-central/rev/f6d082275253
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120530012752
Fixed:
http://hg.mozilla.org/mozilla-central/rev/b5af439f1717
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120530042453
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f6d082275253&tochange=b5af439f1717


Progression window(m-i)
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e0d8fa7fe174
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120529204653
Fixed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d4470d16d227
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120529221453
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e0d8fa7fe174&tochange=d4470d16d227

This seemed to be fixed by Bug 758432
So FIXED, or is there still a problem with X11 basic layers?
I still see this with X11 basic layers.

Bug 758432 was XP_WIN, but that's helpful because the same fix helps here.
(I did still see some occasional glitches, but that might be the window manager.)

Bas talked about a followup bug, so there may be an existing bug on this.
reftest, as it is now at least, won't capture this as it waits after plugin resize for an image of the correct size before processing (capturing) invalidations.
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Attached patch reftestSplinter Review
Actually reftest did capture this.  The plugin paint just needed to be slow enough.

This reftest fails for GL layers.  Even though the window gets drawn correctly, it shouldn't be too much trouble to fix this with GL layers as well for the sake of MozAfterPaint clients.
Attachment #652304 - Flags: review?
Attachment #652304 - Flags: review? → review?(roc)
Virtual methods are rarely inlined, so this just moves the implementation out of the interface declaration, before I modify it in the next patch.
Attachment #652390 - Flags: review?(bas.schouten)
This is different from Bas's original implementation.  This approach snaps a
different rectangle, but afaict is not functionally different in any other
way.  Asking Bas for review in case there was a reason for the other approach
that I missed.
Attachment #652391 - Flags: review?(bas.schouten)
Asking BenWa for review here as the Mac changes are the most significant.
Attachment #652393 - Flags: review?(bgirard)
Now we control the size of the plugin, there's no need to make this code
functional again.
Attachment #652395 - Flags: review?(roc)
Attachment #652393 - Flags: review?(bgirard) → review+
Comment on attachment 652390 [details] [diff] [review]
uninline ImageLayer::ComputeEffectiveTransforms

Obsolete after bug 782372
Attachment #652390 - Attachment is obsolete: true
Attachment #652390 - Flags: review?(bas.schouten)
Comment on attachment 652391 [details] [diff] [review]
implement SCALE_STRETCH through ComputeEffectiveTransforms

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

I don't actually see the code in ComputeEffectiveTransforms being added, am I missing something? :)
Comment on attachment 652391 [details] [diff] [review]
implement SCALE_STRETCH through ComputeEffectiveTransforms

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

I wonder if this will give exactly the right behaviour, but I think it will and if the tests pass I'm happy.
Attachment #652391 - Flags: review?(bas.schouten) → review+
Two changes to ComputeEffectiveTransforms:

1.  The third parameter of the Scale should be unit rather than zero.

2.  When there is no current image, the source size is zero, dividing by which
    leads to unexpected transformations, so don't scale when there is no
    source size.  Drawing a zero-size source will be a no-op whatever the
    transform.
Attachment #661973 - Flags: review?
Attachment #661973 - Flags: review? → review?(bas.schouten)
Comment on attachment 661973 [details] [diff] [review]
implement SCALE_STRETCH through ComputeEffectiveTransforms v1.1

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

::: gfx/layers/ImageLayers.cpp
@@ +42,5 @@
>    // This makes our snapping equivalent to what would happen if our content
>    // was drawn into a ThebesLayer (gfxContext would snap using the local
>    // transform, then we'd snap again when compositing the ThebesLayer).
>    mEffectiveTransform =
> +      SnapTransform(local, sourceRect, nullptr)*

nit: while we're touching this let's add some whitespace before that *
Attachment #661973 - Flags: review?(bas.schouten) → review+
You need to log in before you can comment on or make changes to this bug.