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

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla44
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm7+, firefox44 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(7 attachments, 31 obsolete attachments)

9.76 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.22 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.93 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.28 KB, patch
mstange
: review+
Details | Diff | Splinter Review
Assignee

Description

4 years ago
STR:

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

result: the plugin window fails to position properly
Assignee

Comment 1

4 years ago
Posted file windowed.html (obsolete) —
Assignee

Updated

4 years ago
Blocks: 1127595
Assignee

Updated

4 years ago
Summary: [e10s] Windowed plugins fail to position correctly when the browser menu is displayed → [e10s] Windowed plugins fail to paint correctly when the browser menu is displayed
Assignee

Updated

4 years ago
Component: Plug-ins → Graphics: Layers
Assignee

Updated

4 years ago
Assignee: nobody → jmathies
Assignee

Comment 2

4 years ago
Posted patch invalidation work around (obsolete) — Splinter Review
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.
Assignee

Comment 3

4 years ago
Posted patch will composite patch (obsolete) — Splinter Review
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.
Assignee

Updated

4 years ago
Depends on: 1127794
Assignee

Updated

4 years ago
Blocks: 1138181
Assignee

Updated

4 years ago
No longer blocks: 1127595
Assignee

Comment 4

4 years ago
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?
Flags: needinfo?(matt.woodrow)
Assignee

Updated

4 years ago
Duplicate of this bug: 1131839
Assignee

Updated

4 years ago
Blocks: 1127595
Assignee

Updated

4 years ago
tracking-e10s: --- → m6+
Assignee

Comment 6

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

Comment 7

4 years ago
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.
Whiteboard: gfx-noted
Assignee

Updated

4 years ago
Duplicate of this bug: 1140139
Assignee

Updated

4 years ago
Flags: needinfo?(matt.woodrow)
Assignee

Updated

4 years ago
Blocks: 1148978
Assignee

Updated

4 years ago
Blocks: 1152049
Assignee

Comment 9

4 years ago
Kicking out past aurora uplift: never got to this, cosmetic in nature
Assignee

Comment 10

4 years ago
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.
Assignee

Updated

4 years ago
Blocks: 1157708
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.
Assignee

Comment 12

4 years ago
that's the direction I'm headed here. I'll post wips next week.
Assignee

Comment 13

4 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(karlt)
Assignee

Updated

4 years ago
Attachment #8571036 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8571041 - Attachment is obsolete: true
Assignee

Comment 15

4 years ago
Posted patch wip (obsolete) — Splinter Review
Assignee

Comment 17

4 years ago
> > 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.
Assignee

Comment 18

4 years ago
Posted patch wip (obsolete) — Splinter Review
- added linux support
Attachment #8642560 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8644400 - Attachment is patch: true
Assignee

Comment 19

4 years ago
Posted patch wip (obsolete) — Splinter Review
- cleanup
Attachment #8644400 - Attachment is obsolete: true
Assignee

Comment 20

4 years ago
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.
Assignee

Comment 21

4 years ago
Posted patch wip (obsolete) — Splinter Review
This is working well on Windows, need to test on linux.
Attachment #8644401 - Attachment is obsolete: true
Assignee

Comment 22

4 years ago
This patch adds an additional before composite event we can use to position plugin windows prior to painting.
Attachment #8570766 - Attachment is obsolete: true
Attachment #8645789 - Attachment is obsolete: true
Attachment #8646383 - Flags: review?(matt.woodrow)
Assignee

Comment 23

4 years ago
Assignee

Comment 24

4 years ago
Posted patch part 3 - blocking (obsolete) — Splinter Review
Assignee

Updated

4 years ago
Attachment #8646384 - Flags: review?(roc)
Assignee

Comment 25

4 years ago
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.
Attachment #8646387 - Flags: review?(roc)
Assignee

Updated

4 years ago
No longer blocks: 1152049
Duplicate of this bug: 1152049
Assignee

Updated

4 years ago
No longer blocks: 1148978
Duplicate of this bug: 1148978
Assignee

Updated

4 years ago
No longer blocks: 1127595
Duplicate of this bug: 1127595
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?
Attachment #8646383 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 31

4 years ago
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.
(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 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.
Attachment #8646387 - Flags: review?(roc) → review-
Assignee

Comment 34

4 years ago
(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.
Assignee

Comment 35

4 years ago
(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?
Flags: needinfo?(roc)
Assignee

Comment 37

4 years ago
(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. :)
Assignee

Comment 38

4 years ago
Posted patch part 3 - blocking v.2 (obsolete) — Splinter Review
Attachment #8646387 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8647752 - Flags: review?(roc)
Assignee

Updated

4 years ago
Blocks: 1194661
Assignee

Comment 39

4 years ago
Posted patch part 3 - blocking v.2 (obsolete) — Splinter Review
- comment updates
Attachment #8647752 - Attachment is obsolete: true
Attachment #8647752 - Flags: review?(roc)
Attachment #8648393 - Flags: review?(roc)
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?
Attachment #8648393 - Flags: review?(roc) → review-
Assignee

Comment 41

4 years ago
Posted patch part 3 - blocking v.3 (obsolete) — Splinter Review
> 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.
Attachment #8648393 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8648991 - Flags: review?(roc)
Assignee

Comment 42

4 years ago
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 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
Attachment #8648991 - Flags: review?(roc) → review-
Assignee

Updated

4 years ago
Depends on: 1197538
Assignee

Updated

4 years ago
No longer depends on: 1197538
Assignee

Updated

4 years ago
Blocks: 1196539
Assignee

Comment 44

4 years ago
(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
Assignee

Comment 45

4 years ago
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)
Assignee

Comment 46

4 years ago
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.
Assignee

Comment 47

4 years ago
I'm going to land the first two parts to this so I can use that code in other work.
Keywords: leave-open
Assignee

Comment 48

4 years ago
fixing a bunch of bitrot.
Attachment #8646383 - Attachment is obsolete: true
Attachment #8653491 - Flags: review?(matt.woodrow)
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).
Assignee

Comment 50

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

yes you're right. updating..
Attachment #8653491 - Flags: review?(matt.woodrow) → review-
Assignee

Comment 51

4 years ago
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.
Assignee

Comment 52

4 years ago
Attachment #8648991 - Attachment is obsolete: true
Attachment #8653491 - Attachment is obsolete: true
Attachment #8656145 - Flags: review?(matt.woodrow)
Attachment #8656145 - Flags: review?(matt.woodrow) → review+

Updated

4 years ago
Depends on: 1201660
Assignee

Updated

4 years ago
Depends on: 1201960
Assignee

Comment 55

4 years ago
Posted patch backout of part 1 (obsolete) — Splinter Review
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.
Assignee

Comment 56

4 years ago
Posted patch backout of part 1 (obsolete) — Splinter Review
I had to manually do this, so I'll push to try first.
Attachment #8656145 - Attachment is obsolete: true
Attachment #8658402 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8658404 - Attachment is obsolete: true
Assignee

Comment 60

4 years ago
Posted patch async wip (obsolete) — Splinter Review
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.
Assignee

Comment 61

4 years ago
Posted patch async wip (obsolete) — Splinter Review
This is pretty much complete, just working cleanup and tests.
Attachment #8646384 - Attachment is obsolete: true
Attachment #8659310 - Attachment is obsolete: true
Assignee

Comment 62

4 years ago
Posted patch p1: plugin data caching v.1 (obsolete) — Splinter Review
This adds compositor side caching of plugin data to help cut down on plugin updates.
Attachment #8660170 - Attachment is obsolete: true
Attachment #8661290 - Flags: review?(roc)
Assignee

Comment 63

4 years ago
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.
Attachment #8661293 - Flags: review?(matt.woodrow)
Assignee

Comment 64

4 years ago
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.
Attachment #8661295 - Flags: review?(roc)
Assignee

Comment 65

4 years ago
(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.
Assignee

Comment 66

4 years ago
Posted patch p4: dom scroll hiding v.1 (obsolete) — Splinter Review
Same feature as above but for dom scroll triggered over in content from within nsGfxScrollFrame code.
Attachment #8661301 - Flags: review?(roc)
Assignee

Comment 67

4 years ago
Posted patch tests (obsolete) — Splinter Review
New plugin tests plus some cleanup of the helper routines for existing mochitests in dom/plugins.
Attachment #8661304 - Flags: review?(roc)
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.
Attachment #8661293 - Flags: review?(matt.woodrow) → review+
Assignee

Updated

4 years ago
Attachment #8661290 - Attachment description: plugin data caching v.1 → p1: plugin data caching v.1
Assignee

Updated

4 years ago
Attachment #8661293 - Attachment description: walkthetree changes v.1 → p2: walkthetree changes v.1
Assignee

Updated

4 years ago
Attachment #8661295 - Attachment description: defer composition for plugin update confirmation v.1 → p3: defer composition for plugin update confirmation v.1
Assignee

Updated

4 years ago
Attachment #8661301 - Attachment description: dom scroll hiding v.1 → p4: dom scroll hiding v.1
Assignee

Comment 69

4 years ago
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.
Attachment #8661290 - Flags: review?(roc) → review-
Assignee

Comment 70

4 years ago
Attachment #8661290 - Attachment is obsolete: true
Attachment #8661368 - Flags: review?(roc)
Assignee

Comment 71

4 years ago
Posted patch tests (obsolete) — Splinter Review
additional test updates and tweaks. I wasn't able to unify utils.js and head.js since utils.js gets sourced into html files.
Attachment #8661304 - Attachment is obsolete: true
Attachment #8661304 - Flags: review?(roc)
Attachment #8661399 - Flags: review?(roc)
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?
Flags: needinfo?(jmathies)
Assignee

Comment 73

4 years ago
(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.
Flags: needinfo?(jmathies)
(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.
Assignee

Comment 75

4 years ago
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
Flags: needinfo?(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.
Flags: needinfo?(karlt)
Assignee

Comment 77

4 years ago
(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.
Assignee

Comment 78

4 years ago
Attachment #8661295 - Attachment is obsolete: true
Attachment #8661295 - Flags: review?(roc)
Assignee

Comment 79

4 years ago
added commenting is in this patch.
Attachment #8662471 - Attachment is obsolete: true
Attachment #8662474 - Flags: review?(roc)
Assignee

Comment 80

4 years ago
Posted patch testsSplinter Review
- 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.
Attachment #8661399 - Attachment is obsolete: true
Attachment #8661399 - Flags: review?(roc)
Attachment #8662475 - Flags: review?(roc)
Assignee

Updated

4 years ago
No longer blocks: 1196539
Depends on: 1196539
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
Attachment #8661368 - Flags: review?(roc) → review+
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?
Attachment #8662474 - Flags: review?(roc) → review+
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?
Attachment #8661301 - Flags: review?(roc)
Assignee

Comment 84

4 years ago
(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.
Assignee

Comment 85

4 years ago
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.
Assignee

Comment 86

4 years ago
Posted patch variation of dom scroll (obsolete) — Splinter Review
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?
Attachment #8667381 - Flags: feedback?(roc)
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.
Attachment #8667381 - Flags: feedback?(roc) → review+
Assignee

Comment 88

4 years ago
Posted patch p4: dom scroll hiding v.2 (obsolete) — Splinter Review
Attachment #8661301 - Attachment is obsolete: true
Attachment #8667381 - Attachment is obsolete: true
Assignee

Comment 89

4 years ago
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.
Attachment #8667980 - Flags: review?(roc)
Assignee

Comment 90

4 years ago
Posted patch p5: add a pref (obsolete) — Splinter Review
Assignee

Comment 91

4 years ago
Posted patch p5: add a pref (obsolete) — Splinter Review
Attachment #8667983 - Attachment is obsolete: true
Attachment #8667987 - Flags: review?(roc)
Assignee

Comment 93

4 years ago
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.
Assignee

Updated

4 years ago
Keywords: leave-open
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.
Attachment #8667980 - Flags: review?(roc) → review-
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?
Attachment #8667987 - Flags: review?(roc)
Assignee

Comment 96

4 years ago
(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.
Assignee

Comment 97

4 years ago
updated per comment 94
Attachment #8667980 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8669739 - Flags: review?(roc)
Assignee

Comment 98

4 years ago
(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
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
We also have nsIScrollableFrame::SetTransformingByAPZ which gets called at the start and end of a scroll gesture.
Assignee

Comment 101

4 years ago
(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.
Assignee

Comment 102

4 years ago
updated patch that doesn't have the apz notification code in it.
Attachment #8662474 - Attachment is obsolete: true
Attachment #8669792 - Flags: review+
Assignee

Comment 103

4 years ago
Posted patch p6 - apz scroll handling (obsolete) — Splinter Review
Attachment #8669794 - Flags: review?(roc)
Assignee

Comment 104

4 years ago
Attachment #8667987 - Attachment is obsolete: true
Attachment #8669795 - Flags: review?(roc)
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
Attachment #8669794 - Flags: review?(roc) → review+
Assignee

Comment 106

4 years ago
(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.
Assignee

Comment 107

4 years ago
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?
Attachment #8669794 - Attachment is obsolete: true
Attachment #8670195 - Flags: review?(mstange)
Attachment #8670195 - Flags: review?(mstange) → review+
Assignee

Updated

4 years ago
No longer blocks: 1194661
Assignee

Updated

4 years ago
Duplicate of this bug: 1194661
Assignee

Updated

4 years ago
Duplicate of this bug: 1212484

Updated

4 years ago
Depends on: 1212813

Comment 112

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

Comment 113

4 years ago
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.
(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.
(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.
Depends on: 1213631
Blocks: 1214878
Assignee

Updated

4 years ago
No longer blocks: 1214878
Depends on: 1214878
Assignee

Updated

4 years ago
Depends on: 1213671
Depends on: 1217168

Updated

4 years ago
No longer depends on: 1217168
Blocks: 1193055
Assignee

Comment 116

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

Updated

4 years ago
Depends on: 1232184

Updated

4 years ago
Depends on: 1232302
Assignee

Updated

4 years ago
No longer blocks: 1193055

Comment 117

4 years ago
>>>   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?
Flags: needinfo?(jmathies)
Assignee

Comment 118

4 years ago
(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!
Flags: needinfo?(jmathies)

Comment 119

4 years ago
>>>   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
Flags: needinfo?(jmathies)
Assignee

Comment 120

4 years ago
(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.
Flags: needinfo?(jmathies)

Updated

3 years ago
See Also: → 1245994

Updated

3 years ago
Depends on: 1266684

Updated

3 years ago
See Also: → 1271898
You need to log in before you can comment on or make changes to this bug.