1137944 - [e10s] Windowed plugins fail to paint correctly when the browser menu is displayed

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) load flash test case
2) hit atl to bring up the main window menu

result: the plugin window fails to position properly

Comment 1

[Jim Mathies [:jimm]]

Attachment: windowed.html

Comment 2

[Jim Mathies [:jimm]]

Attachment: invalidation work around

The cause of this is that child windows currently position after parent windows paint. Our parent windows have WS_CLIPCHILDREN so children clip the painting of their parents. This causes a lot of tearing in cases like the shift from a menu display.

This patch is a kludgy work around which invalidates the space the child window leaves after positioning. It fixes the problem but triggers extra paints for each reposition.

Comment 3

[Jim Mathies [:jimm]]

Attachment: will composite patch

Another fix I tried, sending the UpdatePluginConfigurations data to the main thread before we paint. Unfortunately this didn't fix the problem reliably, painting can still finish up early, even on systems without a lot of main thread activity.

Comment 4

[Jim Mathies [:jimm]]

There are two solutions here afaict:

1) Figure out a way to reliably apply plugin geometry updates before the parent window paints its surface.

The 'will composite patch' moves toward this but it's not a perfect fix. Main window painting can still occur before plugin windows move if the main thread is janked. PCompostor's UpdatePluginConfigurations must be async, we don't want to block the compositor thread waiting on the main thread to respond. So generally I don't see a good solution here. (I think we should land 'will composite patch', since I think it is an improvement.)

2) Remove WS_CLIPCHILDREN from parent windows.

This is a pretty cool solution if we can get it to work. Without WS_CLIPCHILDREN, parent windows will paint right over child windows as if they aren't there. So for example when scrolling if a child plugin window doesn't move to a new position before the parent paints, the parent window will render over the area the plugin window is obscuring. Once the plugin updates its position and paint itself everything looks normal.

The trick here is that without WS_CLIPCHILDREN, we are responsible for clipping child windows out of the areas we paint. If we don't "punch a hole" through for every child window, child windows will flicker: the user sees the parent window content painted first, then the child window content.

3) ???

ni'ing matt since he worked on the "hole punching" we did for command buttons on Windows. Matt, any thoughts on option #2? We should have an up to date child window list via widget. I'm wondering if there's a good spot in the layers pipeline where we can jump in and remove any painting for child windows right before we present bits to the screen. Alternatively maybe there's some way to apply a mask at the beginning of the paint cycle to accomplish the same?

Comment 6

[Jim Mathies [:jimm]]

I'm currently using nsIWidget's UpdateThemeGeometries to test this. Chrome seems fine with it but content still flickers. Still investigating.

Comment 7

[Jim Mathies [:jimm]]

There's a problem with option #2 in comment 4. Plugins can paint at any time to their own windows. So in the case where the parent paints then the child window updates position, the plugin has a chance to overwrite what the parent paints to the area the parent will recover.

Comment 9

[Jim Mathies [:jimm]]

Kicking out past aurora uplift: never got to this, cosmetic in nature

Comment 10

[Jim Mathies [:jimm]]

I tried an experiment involving blocking the compositor thread on the main thread waiting for a plugin data updates. Generally it fixed the tearing problem which confirms the theory that this is caused by late positioning of plugin windows post a call to Present. The experimental blocking I implemented uses the existing IPC method of updating plugin data and an win32 event created by the compositor that gets signaled by the main thread when the update finishes. Something like this isn't suitable for release since it can cause deadlocks: the main thread often blocks on the compositor thread during layer tree updates (calls to SendUpdate via PLayerTransaction in layout code). We get into a situation where both threads are blocked waiting on each other. But at least I was able to confirm the cause of the problem.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

FWIW I have a related use case for Fennec where I need to sync something happening on the Java UI thread with something happening on the compositor thread so that everything ends up on the screen in the same frame. It might be worth having an API exposed in the compositor that allows "holding" a composite until some external trigger has completed. This inherently can result in jank if the trigger takes too long but in cases where updates need to happen on other threads there might be no other good option.

Comment 12

[Jim Mathies [:jimm]]

that's the direction I'm headed here. I'll post wips next week.

Comment 13

[Jim Mathies [:jimm]]

karlt, curious if you can answer a linux threading question for me. I'd like to invoke this plugin widget operation from the compositor thread instead of the main thread on linux -

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorChild.cpp#248

The code is currently shared by windows and linux, which is why we bounce this operation from the compositor to the main thread (windows can't do this on the compositor thread) but I'm pretty sure linux can. Need confirmation though, curious if you can answer this?

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Jim Mathies [:jimm] from comment #13)
> I'd like
> to invoke this plugin widget operation from the compositor thread instead of
> the main thread on linux -
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorChild.
> cpp#248
> 
> The code is currently shared by windows and linux, which is why we bounce
> this operation from the compositor to the main thread (windows can't do this
> on the compositor thread) but I'm pretty sure linux can. Need confirmation
> though, curious if you can answer this?

Although X11 window position, size, and clip region can be managed by multiple
threads currently, Gecko accesses these windows by wrapping them in GdkWindow
and nsIWidget objects, which can only be used off main thread if the main
thread is blocked.

Performing the window changes on the compositor thread does seem ideal, but
not so practical.

I don't know why this is an e10s issue and not an omtc issue, as I thought the
same problems would exist for each.

Without much understanding of how this works for omtc, my first guess is that
things would work better than current if the window adjustments happened when
the main thread sends its updates to the compositor thread for compositing.
I'm assuming we can expect the compositor to composite in the next frame after main thread updates.

Comment 15

[Jim Mathies [:jimm]]

Attachment: wip

Comment 16

[Jim Mathies [:jimm]]

windows builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc96d8b1b2d7

Comment 17

[Jim Mathies [:jimm]]

> > I'd like
> > to invoke this plugin widget operation from the compositor thread instead of
> > the main thread on linux -
> > 
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorChild.
> > cpp#248
> > 
> > The code is currently shared by windows and linux, which is why we bounce
> > this operation from the compositor to the main thread (windows can't do this
> > on the compositor thread) but I'm pretty sure linux can. Need confirmation
> > though, curious if you can answer this?
> 
> Although X11 window position, size, and clip region can be managed by
> multiple
> threads currently, Gecko accesses these windows by wrapping them in GdkWindow
> and nsIWidget objects, which can only be used off main thread if the main
> thread is blocked.

Ah yes of course, I forgot about the darn widget. I guess I'll just keep sharing this code between them.

> 
> Performing the window changes on the compositor thread does seem ideal, but
> not so practical.

yep, thanks for the feedback.

Comment 18

[Jim Mathies [:jimm]]

Attachment: wip

- added linux support

Comment 19

[Jim Mathies [:jimm]]

Attachment: wip

- cleanup

Comment 20

[Jim Mathies [:jimm]]

Testing on linux exposed a problem here, the main thread can remain in MessageChannel Send() through multiple cycles of the compositor thread. In my current wip I free a waiting compositor thread when the main thread enters Send, but what I really need to do is blacklist waiting the entire time the main thread is is Send. Working on an update today.

Comment 21

[Jim Mathies [:jimm]]

Attachment: wip

This is working well on Windows, need to test on linux.

Comment 22

[Jim Mathies [:jimm]]

Attachment: part 1 - add before and after composite events

This patch adds an additional before composite event we can use to position plugin windows prior to painting.

Comment 23

[Jim Mathies [:jimm]]

Attachment: part 2 - simplify the hide widget api

Comment 24

[Jim Mathies [:jimm]]

Attachment: part 3 - blocking

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8646387 [details] [diff] [review]
part 3 - blocking

This is the blocking part of the patch. The compositor thread will block on the main thread if there are new layer tree updates with plugin data to promote to the main thread side. This wait will abort if the main thread enters a sync ipc send since this can result in deadlocks, or complete the update through a normal ipc message or through an 'interrupt' in main thread processing via CheckForCompositorPluginUpdates. A lock is also used here to avoid a situation where the compositor thread loops around and passes through UpdatePluginWindowState twice while the main thread is in an ipc Send.

Comment 26

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58f57209274c

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8646383 [details] [diff] [review]
part 1 - add before and after composite events

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1179,5 @@
>      return;
>    }
>  
> +  // Takes care of fireing 'will' and 'did' composite events.
> +  AutoFireCompositorEvents afce(this);

Should this be before the CanComposite check to preserve the existing behaviour? If not, why is this change ok?

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 8646383 [details] [diff] [review]
part 1 - add before and after composite events

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1179,5 @@
>      return;
>    }
>  
> +  // Takes care of fireing 'will' and 'did' composite events.
> +  AutoFireCompositorEvents afce(this);

I was curious about this as well. It looked like this event wasn't needed, and I haven't seen any fall out in testing. I can move it up though.

Comment 32

[Matt Woodrow (:mattwoodrow)]

(In reply to Jim Mathies [:jimm] from comment #31)
> Comment on attachment 8646383 [details] [diff] [review]
> part 1 - add before and after composite events
> 
> Review of attachment 8646383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +1179,5 @@
> >      return;
> >    }
> >  
> > +  // Takes care of fireing 'will' and 'did' composite events.
> > +  AutoFireCompositorEvents afce(this);
> 
> I was curious about this as well. It looked like this event wasn't needed,
> and I haven't seen any fall out in testing. I can move it up though.

It'd be very rare for it to matter. If something paused the compositor (I think android does this?), and then also decided to wait on the 'MozAfterPaint' event then we'd be stuck.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8646387 [details] [diff] [review]
part 3 - blocking

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

Also I shouldn't review the IPDL stuff, so split it out into a separate patch.

::: ipc/glue/MessageChannel.cpp
@@ +248,5 @@
> +
> +#if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
> +        if (frame.IsOutgoingSync() && NS_IsMainThread() && that.Listener() &&
> +            PCompositorMsgStart == that.Listener()->GetProtocolTypeId()) {
> +            mozilla::layers::MessageChannelEnteringSyncSend();

This seems like a pretty big layering violation. I think we need to abstract this out into a general mechanism for observing sync IPCs.

Comment 34

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 8646387 [details] [diff] [review]
> part 3 - blocking
> 
> Review of attachment 8646387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also I shouldn't review the IPDL stuff, so split it out into a separate
> patch.
> 
> ::: ipc/glue/MessageChannel.cpp
> @@ +248,5 @@
> > +
> > +#if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
> > +        if (frame.IsOutgoingSync() && NS_IsMainThread() && that.Listener() &&
> > +            PCompositorMsgStart == that.Listener()->GetProtocolTypeId()) {
> > +            mozilla::layers::MessageChannelEnteringSyncSend();
> 
> This seems like a pretty big layering violation. I think we need to abstract
> this out into a general mechanism for observing sync IPCs.

I considered implementing another RAII helper here, similar to CxxStackFrame which I'm currently using. If that doesn't fit with what you're looking for let me know.

Comment 35

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #34)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
>
> > This seems like a pretty big layering violation. I think we need to abstract
> > this out into a general mechanism for observing sync IPCs.
> 
> I considered implementing another RAII helper here, similar to CxxStackFrame
> which I'm currently using. If that doesn't fit with what you're looking for
> let me know.

I think I get what you're asking for here - essentially something built into ipdl wrapper code that provides a virtual call that the object can override to receive notices regarding entering and leaving sync sends?

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah

Comment 37

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Yeah

Turns out we already had support for this in MessageChannel, I just didn't know about it. :)

Comment 38

[Jim Mathies [:jimm]]

Attachment: part 3 - blocking v.2

Comment 39

[Jim Mathies [:jimm]]

Attachment: part 3 - blocking v.2

- comment updates

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8648393 [details] [diff] [review]
part 3 - blocking v.2

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +90,5 @@
>  
> +#if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
> +// Plugin window data structure and sharing lock
> +extern StaticAutoPtr<mozilla::Monitor> sPluginDataLock;
> +extern StaticAutoPtr<PluginUpdateData> sPluginData;

All the variables in this block need much better comments describing what they're for and the invariants around them.

@@ +2186,5 @@
> +          sPluginData->visibleRegion = visibleRegion;
> +        }
> +
> +        // Event set by the main thread when sPluginData gets flushed.
> +        sUpdateHandle->Reset();

How is it safe to do this outside the lock?

Comment 41

[Jim Mathies [:jimm]]

Attachment: part 3 - blocking v.3

> All the variables in this block need much better comments describing what
> they're for and the invariants around them.

updated.

> @@ +2186,5 @@
> > +          sPluginData->visibleRegion = visibleRegion;
> > +        }
> > +
> > +        // Event set by the main thread when sPluginData gets flushed.
> > +        sUpdateHandle->Reset();
> 
> How is it safe to do this outside the lock?

It isn't so I moved it up. There was a brief window of time here where the main thread might race on the compositor after signaling sUpdateHandle.

Comment 42

[Jim Mathies [:jimm]]

After looking this over, I think sSendHandle needs to be reset manually as well on the child side while the compositor thread holds the lock on sSyncSendMessageLock. Will do some testing and post back tomorrow.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8648991 [details] [diff] [review]
part 3 - blocking v.3

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

::: gfx/layers/ipc/CompositorChild.cpp
@@ +131,5 @@
> + * sSyncSendMessageLock - the gate that controls access to
> + * sMainThreadIsInSyncSend.
> + */
> +bool sMainThreadIsInSyncSend;
> +StaticAutoPtr<mozilla::Monitor> sSyncSendMessageLock;

Can we simplify this a bit by sharing the same lock with sPluginDataLock?

Also is there a reason why we're using WaitableEvents here instead of Wait() on the Monitor as we usually would? That might be easier to understand than mixing locks with these WaitableEvents.

@@ +419,5 @@
>    nsIWidget::UpdateRegisteredPluginWindowVisibility((uintptr_t)parent, visiblePluginIds);
> +}
> +
> +static void
> +InitPluginData()

Document that this is always called on the main thread.

@@ +443,5 @@
> +
> +// Main thread IPC Send inturrupt callback, called from MessageChannel prior
> +// to a sync send to parent in PCompositor. This callback frees the compositor
> +// thread if it is waiting on the main thread to update plugin information but
> +// does not update plugins. This can result is drawing anomalies in content. 

Fix trailing whitespace

Comment 44

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Comment on attachment 8648991 [details] [diff] [review]
> part 3 - blocking v.3
> 
> Review of attachment 8648991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +131,5 @@
> > + * sSyncSendMessageLock - the gate that controls access to
> > + * sMainThreadIsInSyncSend.
> > + */
> > +bool sMainThreadIsInSyncSend;
> > +StaticAutoPtr<mozilla::Monitor> sSyncSendMessageLock;
> 
> Can we simplify this a bit by sharing the same lock with sPluginDataLock?

Looks like it.

> Also is there a reason why we're using WaitableEvents here instead of Wait()
> on the Monitor as we usually would? That might be easier to understand than
> mixing locks with these WaitableEvents.

WaitableEvents give you the ability to set an event and have another thread pass through the lock after the event is set. I'll try rewriting this to use a monitor but I'm not sure it will work: we need to be sure the compositor thread never waits when the main thread is in a sync send. Monitors can't track thread state afaik.

> 
> @@ +419,5 @@
> >    nsIWidget::UpdateRegisteredPluginWindowVisibility((uintptr_t)parent, visiblePluginIds);
> > +}
> > +
> > +static void
> > +InitPluginData()
> 
> Document that this is always called on the main thread.
> 
> @@ +443,5 @@
> > +
> > +// Main thread IPC Send inturrupt callback, called from MessageChannel prior
> > +// to a sync send to parent in PCompositor. This callback frees the compositor
> > +// thread if it is waiting on the main thread to update plugin information but
> > +// does not update plugins. This can result is drawing anomalies in content. 
> 
> Fix trailing whitespace

Comment 45

[Jim Mathies [:jimm]]

This is what I'm trying to put together, but so far I haven't found a clean way to do it - 

thread a:
do {
  {
    // while thread a is in here, thread b can't wait, and
    // if it's waiting it should get freed. must be atomic.
    dont_wait_on_a(monitor)
    doStuffForUnknownPeriodOfTime()
  }
  {
    lock(datalock)
    // access some shared data
  }
  // tell thread b to stop waiting if it's waiting right now
  monitor.notify()
} while (1)


thread b:
do {
  {
    lock(datalock)
    // fill some shared data
  }
  wait_for_a(monitor)
} while (1)

Comment 46

[Jim Mathies [:jimm]]

It seems to me a manually reset WaitableEvent is the best bet here, I don't see a way to do the dont_wait_on_a(monitor) bits without it. I was originally using two of these and two locks, pretty sure I can cook that down to use fewer objects.

Comment 47

[Jim Mathies [:jimm]]

I'm going to land the first two parts to this so I can use that code in other work.

Comment 48

[Jim Mathies [:jimm]]

Attachment: part 1 - add before and after composite events

fixing a bunch of bitrot.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8653491 [details] [diff] [review]
part 1 - add before and after composite events

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1103,5 @@
>        return;
>      }
>    }
>  
> +  afce.WillComposite();

Won't this result in SendDidComposite being called (for CompositorParent) *before* we actually composite since we don't check aType in that function?

It doesn't matter all that much for triggering the MozAfterPaint event, but it will mess up the timestamps being sent (used for profiling markers).

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 8653491 [details] [diff] [review]
part 1 - add before and after composite events

yes you're right. updating..

Comment 51

[Jim Mathies [:jimm]]

Also, I spoke to billm, he suggested a different approach to the blocking here which I'm going to experiment with. Rather than use locks and actually block the compositor thread on the other, I'll use an async event and block composition/painting on the compositor thread waiting on an async confirmation event from the main thread. Essentially we'll put painting on hold until the main thread catches up, with some sort of reasonable time out. If the time it takes to do this is too long, we might throw up the spinner in place of content. Not sure how it'll come together, but it seems safer than the use of actual locks.

Comment 52

[Jim Mathies [:jimm]]

Attachment: part 1 - add before and after composite events v.2

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0325cf8f3cda
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4711fa5f734

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/0325cf8f3cda
https://hg.mozilla.org/mozilla-central/rev/b4711fa5f734

Comment 55

[Jim Mathies [:jimm]]

Attachment: backout of part 1

I'm going to back this part out. I don't think I'll need it, and it seems to have messed up some compositor timing metrics.

Comment 56

[Jim Mathies [:jimm]]

Attachment: backout of part 1

I had to manually do this, so I'll push to try first.

Comment 57

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff6520544ac2

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb965937e24

Comment 59

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

https://hg.mozilla.org/mozilla-central/rev/5bb965937e24

Comment 60

[Jim Mathies [:jimm]]

Attachment: async wip

Async attempt wip. This generally works. The performance when of the compositor is pretty bad though when deferring is on so I've tried to avoid it as much as possible. One trick I'm using is to hide plugins when scrolling. I have this worked out for apz scroll wheel scroll, but not for dom scroll. Not sure if I need to get that working yet.

Comment 61

[Jim Mathies [:jimm]]

Attachment: async wip

This is pretty much complete, just working cleanup and tests.

Comment 62

[Jim Mathies [:jimm]]

Attachment: p1: plugin data caching v.1

This adds compositor side caching of plugin data to help cut down on plugin updates.

Comment 63

[Jim Mathies [:jimm]]

Attachment: p2: walkthetree changes v.1

Updating AutoResolveRefLayers / WalkTheTree to handle plugin updates. This solves a timing problem the old code had with plugin updates being tied to layer tree updates vs. composition.

Comment 64

[Jim Mathies [:jimm]]

Attachment: p3: defer composition for plugin update confirmation v.1

After a plugin update is sent async, the compositor needs to wait for the main thread to acknowledge the update completed. The acknowledgment comes back async as well. This replaces the buggy sync thread blocking stuff I was originally attempting.

Comment 65

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #64)
> Created attachment 8661295 [details] [diff] [review]
> defer composition for plugin update confirmation v.1
> 
> After a plugin update is sent async, the compositor needs to wait for the
> main thread to acknowledge the update completed. The acknowledgment comes
> back async as well. This replaces the buggy sync thread blocking stuff I was
> originally attempting.

This code also has plugin window hiding in it. Sorry about that I though I split this out.  Basically what this does is hide visible plugin windows at the start of an async apz scroll and show them when complete. This avoids visible lag in plugin window positioning. I'll file a follow up on polishing this up, maybe through a screen grab we paint in place of the plugin window during scroll.

Comment 66

[Jim Mathies [:jimm]]

Attachment: p4: dom scroll hiding v.1

Same feature as above but for dom scroll triggered over in content from within nsGfxScrollFrame code.

Comment 67

[Jim Mathies [:jimm]]

Attachment: tests

New plugin tests plus some cleanup of the helper routines for existing mochitests in dom/plugins.

Comment 68

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8661293 [details] [diff] [review]
p2: walkthetree changes v.1

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +151,5 @@
> +                       didResolvePlugins);
> +  if (aHasRemoteContent) {
> +    *aHasRemoteContent = hasRemoteContent;
> +  }
> +  if (aResolvePlugins != nullptr) {

No need for '!= nullptr' here and above, or do the same for aHasRemoteContent.

Comment 69

[Jim Mathies [:jimm]]

Comment on attachment 8661290 [details] [diff] [review]
p1: plugin data caching v.1

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +2022,5 @@
> +    }
> +
> +    // check for plugin data changes
> +    for (uint32_t idx = 0; idx < lts.mPluginData.Length(); idx++) {
> +      if (!(mCachedPluginData[idx] == lts.mPluginData[idx])) {

the address sanitizer picked up a nice little bug here - I failed to compare the lengths, so sometimes idx was beyond the bounds mCachedPluginData. New patch forthcoming.

Comment 70

[Jim Mathies [:jimm]]

Attachment: p1: plugin data caching v.2

Comment 71

[Jim Mathies [:jimm]]

Attachment: tests

additional test updates and tweaks. I wasn't able to unify utils.js and head.js since utils.js gets sourced into html files.

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you document somewhere, preferably in code, what the overall strategy is here?

Also, can you summarize somewhere what exactly this plugin hiding does, and why it's better than the alternatives?

Comment 73

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> Can you document somewhere, preferably in code, what the overall strategy is
> here?

Sure, I'll add something and post a patch today.

> Also, can you summarize somewhere what exactly this plugin hiding does, and
> why it's better than the alternatives?

Thread locking requires we get a locking mechanism working with existing ipdl mechanics which breaks ipdl rules about blocking. Based on my experimentation this isn't easy to do. What we'd need is illustrated by this pseudo code:

thread compositor:
do {
  do_stuff()
  process_sync_requests_from_m()

  {
    lock(data)
    // fill some shared data
    data = x
  }
  wait_for_ack_from_m(monitor)
} while (1)

thread main:
do {
  do_stuff()
  {
    lock(data)
    // access shared data
    y = data
    ack_c(monitor)
  }
  {
    // while thread m is in here, thread c can't wait, and
    // if it's waiting it should get freed.
    dont_wait_on_thread_m(monitor)
    // c can loop multiple times while m is in here:
    sync_sends_to_c()
  }
} while (1)


My main issue here has been the functionality needed for 'dont_wait_on_thread_m', which needs to apply across multiple loops of the compositor thread. There are corner cases here in the implementation where the two threads race and you end up with a deadlock. There's also no clean cross platform way to do this. Windows has some synchronization objects that can help facilitate it, but linux support lacks.

I chatted with billm who understands ipdl really well about the above scenario, he pushed the idea that I should use post + acknowledge for this instead to avoid the deadlock risk. With an async approach though you get a lot of lag during scrolling, so we came up with the idea of hiding plugin windows during long scroll operations. Hence the current approach. Post + acknowledge also solves the core problem this bug was originally supposed to fix - making sure plugin window updates happen before painting. The thread deadlock approach didn't accomplish that fix 100% because there are times where you have to discard requests for plugin window updates to avoid a deadlock. So the async approach actually does a better job of fixing the original bug.

Once we get window capture or a backdrop implemented I think this will be a totally acceptable approach that doesn't risk deadlocks. Its greatest risk is long periods of painting suppression due to main thread jank, but really, if the main thread is lagged, the browser isn't going to be operating properly anyway. IMO this is a different problem which is primarily tied to legacy addons. Obviously we're working on that too.

Comment 74

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jim Mathies [:jimm] from comment #73)
> so we came up with the idea of hiding plugin
> windows during long scroll operations.

FWIW on Fennec we have a similar problem with Java UI widgets (which move on the Java UI thread) needing to be kept in sync with the Gecko compositor thread. There too (for example the form input assistant popup that shows up on text fields) we just hide the UI widget while the user is actively scrolling.

Comment 75

[Jim Mathies [:jimm]]

Hey karl, do you have any ideas on how to implement a window visibility check for gtk? Searching for an api I can use isn't turning anything up.. basically I'm looking for the equivalent of win32's IsWindowVisible.

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/nptest_windows.cpp#595

Comment 76

[Karl Tomlinson (:karlt)]

gdk_window_is_viewable() is most similar to IsWindowVisible(HWND).
https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-is-viewable

gtk/nsWindow also keeps track of "visibility" in mIsFullyObscured which can be affected by the position and z-order of non-ancestor windows.

Comment 77

[Jim Mathies [:jimm]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #76)
> gdk_window_is_viewable() is most similar to IsWindowVisible(HWND).
> https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-is-
> viewable
> 
> gtk/nsWindow also keeps track of "visibility" in mIsFullyObscured which can
> be affected by the position and z-order of non-ancestor windows.

Hmm, didn't have any luck getting this to work, it gave false positives. The tests run on windows and I've confirmed the hiding is working on gtk manually, so I think we're ok.

Comment 78

[Jim Mathies [:jimm]]

Attachment: p3: defer composition for plugin update confirmation v.2

Comment 79

[Jim Mathies [:jimm]]

Attachment: p3: defer composition for plugin update confirmation v.2

added commenting is in this patch.

Comment 80

[Jim Mathies [:jimm]]

Attachment: tests

- made the scroll tests more reliable by slowing down smooth scroll through temporary pref changes, which insures scrolling is taking place when I check the test plugin visible state.

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8661368 [details] [diff] [review]
p1: plugin data caching v.2

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

::: gfx/layers/ipc/CompositorParent.h
@@ +387,2 @@
>     */
> +  void UpdatePluginWindowState(uint64_t aId);

This should return bool

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8662474 [details] [diff] [review]
p3: defer composition for plugin update confirmation v.2

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1062,5 @@
>  #if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
> +  /*
> +   * AutoResolveRefLayers handles two tasks related to Windows and Linux
> +   * plugin window management:
> +   * 1) calculating if we have remove content in the view. If we do not have

"remote content"

::: gfx/layers/ipc/CompositorParent.h
@@ +395,5 @@
> +  void ScheduleHideAllPluginWindowsAPZ();
> +  void ShowAllPluginWindowsAPZ();
> +  void HideAllPluginWindowsAPZ();
> +  void ShowAllPluginWindows();
> +  void HideAllPluginWindows();

Explain the difference between the APZ and non-APZ versions here.

@@ +502,5 @@
>    nsIntRegion mPluginsLayerVisibleRegion;
>    nsTArray<PluginWindowData> mCachedPluginData;
> +  // indicated if plugin window visibility and metric updates are currently
> +  // being defered due to a scroll operation.
> +  bool mDeferPluginWindows;

I'd prefer a slightly more explanatory name here and in mAPZPluginDeferment below.

mHidingPluginWindowsDuringScrolling, mHidingPluginWindowsDuringAPZScrolling?

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8661301 [details] [diff] [review]
p4: dom scroll hiding v.1

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

Did you consider hiding plugins during DOM scrolling entirely in the content process? Would that be simpler?

Do we need to modify nsPluginFrame::IsOpaque or somewhere around there to ensure that content behind the plugin is rendered, so when we hide the plugin asynchronously we have something behind it to render?

I'm still not entirely comfortable with the idea of hiding plugins during scrolling. Maybe it's the right thing to do, but I think we should be prepared to change course if necessary. Put it behind a pref maybe?

Comment 84

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> Comment on attachment 8661301 [details] [diff] [review]
> p4: dom scroll hiding v.1
> 
> Review of attachment 8661301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you consider hiding plugins during DOM scrolling entirely in the content
> process? Would that be simpler?

No I didn't, I'll take a look.

> Do we need to modify nsPluginFrame::IsOpaque or somewhere around there to
> ensure that content behind the plugin is rendered, so when we hide the
> plugin asynchronously we have something behind it to render?

I assumed we were rendering the content underneath when the window was hidden and relying on os behavior to clip that part of painting out. It didn't occur to me we might have our own avoidance mechanism (for perf reasons I'm guessing?). Improving what displays in place of the plugin was going to be done in a follow up - maybe a screen grab of the plugin window, or some sort of backdrop added by front end behind the bounds of the window. I'll take a look at IsOpaque, maybe what I'm seeing there is just a result of our own avoidance.. and if I turn that off we won't need the screen grab or backdrop.

> I'm still not entirely comfortable with the idea of hiding plugins during
> scrolling. Maybe it's the right thing to do, but I think we should be
> prepared to change course if necessary. Put it behind a pref maybe?

I'll add a pref. I have the old blocking patches still and I be happy to discuss more if you want. Maybe we could get together on vidyo with billm once he's back from pto and hash it out more. Getting thread blocking working with ipdl mechanics is messy but maybe there's some way to do it that doesn't have deadlock risk.

Comment 85

[Jim Mathies [:jimm]]

Also, we could meet up at mozlando. With the new release criteria stuff tied to e10s we'll likely be holding out on aurora past the next summit.

Comment 86

[Jim Mathies [:jimm]]

Attachment: variation of dom scroll

Hey roc, this is a variation of dom scroll support that is isolated to the content process. This generally works for keyboard shortcut scrolling except that occasionally plugin windows don't reappear after a scroll. I think I need to trigger a final paint+composition, but I'm not sure how to go about it.. curious if you have any thoughts?

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8667381 [details] [diff] [review]
variation of dom scroll

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1991,5 @@
> +      childFrame->SetScrollVisibility(aVisibleState);
> +    } else {
> +      RecurseNotifyPluginFrames(childFrame, aVisibleState);
> +    }
> +  }

You can walk the plugins more efficiently by following what PresShell::Freeze does.

::: layout/generic/nsPluginFrame.h
@@ +202,5 @@
> +  /**
> +   * XXX
> +   */
> +  void SetScrollVisibility(bool aState) override {
> +    mIsHiddenDueToScroll = aState;

You want to call SchedulePaint here if the state has changed.

Comment 88

[Jim Mathies [:jimm]]

Attachment: p4: dom scroll hiding v.2

Comment 89

[Jim Mathies [:jimm]]

Comment on attachment 8667980 [details] [diff] [review]
p4: dom scroll hiding v.2

This appears to be working reliably and is passing all my tests.

Comment 90

[Jim Mathies [:jimm]]

Attachment: p5: add a pref

Comment 91

[Jim Mathies [:jimm]]

Attachment: p5: add a pref

Comment 92

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c8944c48677

Comment 93

[Jim Mathies [:jimm]]

Comment on attachment 8667980 [details] [diff] [review]
p4: dom scroll hiding v.2

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

::: dom/base/nsIObjectLoadingContent.idl
@@ +191,5 @@
> +
> +  /**
> +   * Helper for hiding windowed plugins during non-apz scroll operations.
> +   */
> +  [noscript] attribute bool scrollVisibility;

I think I might want to change the name of this. As is a value of true clips the plugin, false displays it. I could make it more generic -

'shouldClip', 'windowedVisibility', 'scrollActive', 'hidePlugin'

something like this.

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8667980 [details] [diff] [review]
p4: dom scroll hiding v.2

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1980,5 @@
> +NotifyPluginFramesCallback(nsISupports* aSupports, void* aFlag)
> +{
> +  nsCOMPtr<nsIObjectLoadingContent> olc(do_QueryInterface(aSupports));
> +  if (olc) {
> +    olc->SetScrollVisibility(aFlag != nullptr);

Rather than change nsIObjectLoadingContent, I would do_QueryInterface to nsIContent, and if that works, call GetPrimaryFrame and do_QueryFrame to nsPluginFrame* and call SetScrollVisibility directly.

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8667987 [details] [diff] [review]
p5: add a pref

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +2167,5 @@
>  
>  void
>  CompositorParent::ScheduleShowAllPluginWindowsAPZ()
>  {
> +  if (!gfxPrefs::HidePluginsForScroll()) {

Why do we need any APZ changes? Surely the change in NotifyPluginFrames is enough?

Comment 96

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #95)
> Comment on attachment 8667987 [details] [diff] [review]
> p5: add a pref
> 
> Review of attachment 8667987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +2167,5 @@
> >  
> >  void
> >  CompositorParent::ScheduleShowAllPluginWindowsAPZ()
> >  {
> > +  if (!gfxPrefs::HidePluginsForScroll()) {
> 
> Why do we need any APZ changes? Surely the change in NotifyPluginFrames is
> enough?

Didn't work that way, apz didn't trigger these events. Maybe there's a scroll path in nsGfxScrollFrame that I need to monitor, will take a look.

Comment 97

[Jim Mathies [:jimm]]

Attachment: p4: dom scroll hiding v.3

updated per comment 94

Comment 98

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #96)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #95)
> > Why do we need any APZ changes? Surely the change in NotifyPluginFrames is
> > enough?
> 
> Didn't work that way, apz didn't trigger these events. Maybe there's a
> scroll path in nsGfxScrollFrame that I need to monitor, will take a look.

The problem here is that apz triggers small increments of scroll type nsIScrollableFrame::INSTANT. Looking at the stack, it should be pretty easy to identify apz related scroll in nsGfxScrollFrame, but not the macro scroll operation the apz manager is running without adding start and end events sent from chrome. It looks like controlling this on the chrome side will be simpler. Any ideas roc?

content scroll stack:

mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint aScrollPosition, nsIScrollableFrame::ScrollMode aMode, nsIAtom * aOrigin, const nsRect * aRange, nsIScrollbarMediator::ScrollSnapMode aSnap) Line 2084
mozilla::ScrollFrameHelper::ScrollToCSSPixelsApproximate(const mozilla::gfx::PointTyped<mozilla::CSSPixel,float> & aScrollPosition, nsIAtom * aOrigin) Line 2049
mozilla::layers::ScrollFrameTo(nsIScrollableFrame * aFrame, const mozilla::gfx::PointTyped<mozilla::CSSPixel,float> & aPoint, bool & aSuccessOut) Line 100
mozilla::layers::ScrollFrame(nsIContent * aContent, mozilla::layers::FrameMetrics & aMetrics) Line 126
mozilla::layers::APZCCallbackHelper::UpdateRootFrame(mozilla::layers::FrameMetrics & aMetrics) Line 240
mozilla::dom::TabChildBase::ProcessUpdateFrame(const mozilla::layers::FrameMetrics & aFrameMetrics) Line 295
mozilla::dom::TabChildBase::UpdateFrameHandler(const mozilla::layers::FrameMetrics & aFrameMetrics) Line 274
mozilla::dom::PBrowserChild::OnMessageReceived(const IPC::Message & msg__) Line 3000

Comment 99

[Kartikaya Gupta (email:[e-mail])]

The apz-triggered scrolls should have an "origin" value of nsGkAtoms::apz [1]; that might help identify them. We have other pieces of code that assume all APZ scrolls have this origin value, so that we can properly merge APZ-driven scrolls with JS-drive scrollTo calls and such.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=25e572cce005#99

Comment 100

[Markus Stange [:mstange]]

We also have nsIScrollableFrame::SetTransformingByAPZ which gets called at the start and end of a scroll gesture.

Comment 101

[Jim Mathies [:jimm]]

(In reply to Markus Stange [:mstange] from comment #100)
> We also have nsIScrollableFrame::SetTransformingByAPZ which gets called at
> the start and end of a scroll gesture.

This looks like it might work. AFAICT it is only called twice per macro scroll operation.

Comment 102

[Jim Mathies [:jimm]]

Attachment: p3: defer composition for plugin update confirmation v.3

updated patch that doesn't have the apz notification code in it.

Comment 103

[Jim Mathies [:jimm]]

Attachment: p6 - apz scroll handling

Comment 104

[Jim Mathies [:jimm]]

Attachment: p5: add a pref v.2

Comment 105

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8669794 [details] [diff] [review]
p6 - apz scroll handling

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

::: layout/generic/nsGfxScrollFrame.h
@@ +360,5 @@
>        // because we have special behaviour for it when APZ scrolling is active.
>        mOuter->SchedulePaint();
>      }
> +    // Update windowed plugin visibility in response to apz scrolling events.
> +    NotifyPluginFrames(aTransforming == true ? BEGIN : END);

aTransforming ? BEGIN : END

Comment 106

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #101)
> (In reply to Markus Stange [:mstange] from comment #100)
> > We also have nsIScrollableFrame::SetTransformingByAPZ which gets called at
> > the start and end of a scroll gesture.
> 
> This looks like it might work. AFAICT it is only called twice per macro
> scroll operation.

Hmm, I just randomly came across a web site where this doesn't appear to work reliably. Need to investigate.

Comment 107

[Jim Mathies [:jimm]]

Attachment: p6 - apz scroll handling v.2

This required a slight tweak so that we ignore 'inner' dom scroll calls to NotifyPluginFrames while apz transformations are taking place.

marcus, any chance you could r+ this minor update?

Comment 108

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1639988075d
https://hg.mozilla.org/integration/mozilla-inbound/rev/027a2d0e72b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8916358ba6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d78f38ee2f65
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47194bc9aaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c53ef094709
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e281f04f27

Comment 110

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

https://hg.mozilla.org/mozilla-central/rev/a1639988075d
https://hg.mozilla.org/mozilla-central/rev/027a2d0e72b5
https://hg.mozilla.org/mozilla-central/rev/ef8916358ba6
https://hg.mozilla.org/mozilla-central/rev/d78f38ee2f65
https://hg.mozilla.org/mozilla-central/rev/f47194bc9aaa
https://hg.mozilla.org/mozilla-central/rev/6c53ef094709
https://hg.mozilla.org/mozilla-central/rev/e1e281f04f27

Comment 112

[2ljpswpih]

My Nightly doesn't see any updates since 7th October. Bug still persists on www.uz.zgora.pl

Comment 113

[2ljpswpih]

After today update element is visible and hidden (but not smoothly) while scrolling. I don't think that this is good way to fix this bug. With every scroll element is not visible and it doesn't look good.

Comment 114

[u279076]

(In reply to to_du from comment #113)
> After today update element is visible and hidden (but not smoothly) while
> scrolling. I don't think that this is good way to fix this bug. With every
> scroll element is not visible and it doesn't look good.

Please file a new bug report describing the problem in greater detail so it can be investigated separately.

Comment 115

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #114)
> (In reply to to_du from comment #113)
> > After today update element is visible and hidden (but not smoothly) while
> > scrolling. I don't think that this is good way to fix this bug. With every
> > scroll element is not visible and it doesn't look good.
> 
> Please file a new bug report describing the problem in greater detail so it
> can be investigated separately.

Just to close the loop here, over on bug 1212484 comment 14 Jim provides a better explanation. Please contain further conversation to that bug to avoid duplicating effort.

Comment 116

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #99)
> The apz-triggered scrolls should have an "origin" value of nsGkAtoms::apz
> [1]; that might help identify them. We have other pieces of code that assume
> all APZ scrolls have this origin value, so that we can properly merge
> APZ-driven scrolls with JS-drive scrollTo calls and such.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/
> APZCCallbackHelper.cpp?rev=25e572cce005#99

botond pointed out to be today that this event is sent async to the content process, in which case plugin updates can lag behind visible scrolling. I think we need to switch back to the method I originally had which had apz controlling plugin visibility on the browser side.

Comment 117

[arni2033]

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160101030330
I confirm that it no longer happens with str in comment 0 (actually, I still see some blinking for about 0.5s, but then flash redraws in a normal way)
But if I press Alt/F10 during tabs animation (when I close a tab), then flash still fails to redraw
> screenshot:   https://www.dropbox.com/s/ebi7f0jjnrjzh6o/bug%201137944%20-%20confirmation.png?dl=0
// Also, there's an issue when entering/exiting F11 fullscreen.
Is this according to the plan? Should I report this separately with detailed STR or reopen this bug?

Comment 118

[Jim Mathies [:jimm]]

(In reply to arni2033 from comment #117)
> > screenshot:   https://www.dropbox.com/s/ebi7f0jjnrjzh6o/bug%201137944%20-%20confirmation.png?dl=0
> // Also, there's an issue when entering/exiting F11 fullscreen.
> Is this according to the plan? Should I report this separately with detailed
> STR or reopen this bug?

Hmm, I'm not seeing an issue with fullscreen and flash but if you are please file a new bug with str. Thanks!

Comment 119

[arni2033]

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160118030338
> https://dl.dropboxusercontent.com/s/iykw8c6uhc9k6d7/bug%201137944%20-%20comment%20119.png?dl=0
I was wrong, comment 0 isn't fixed at all. I can reproduce it without animation ans stuff, with
HWA disabled (even with APZ disabled) on attachment 8675621 [details] from bug 1148978.
I think this should be reopened. OR another bug should be used to track the progress on fixing this

Comment 120

[Jim Mathies [:jimm]]

(In reply to arni2033 from comment #119)
> >>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160118030338
> > https://dl.dropboxusercontent.com/s/iykw8c6uhc9k6d7/bug%201137944%20-%20comment%20119.png?dl=0
> I was wrong, comment 0 isn't fixed at all. I can reproduce it without
> animation ans stuff, with
> HWA disabled (even with APZ disabled) on attachment 8675621 [details] from
> bug 1148978.
> I think this should be reopened. OR another bug should be used to track the
> progress on fixing this

I'm nor seeing this and I'm confident the work than landed here fixed the original positioning problem. Please file a new bug with specific steps, and if you can please post your about:support too.