Bug 107701 - Canvas and DOM go out of sync
Summary: Canvas and DOM go out of sync
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: sugoi
URL: http://jsfiddle.net/pV2ud/10/
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 09:46 PST by sugoi
Modified: 2013-03-04 14:18 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.39 KB, patch)
2013-01-23 09:53 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (7.90 KB, patch)
2013-01-29 11:12 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2013-01-30 08:27 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (5.61 KB, patch)
2013-01-30 13:34 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2013-01-31 06:22 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (5.53 KB, patch)
2013-01-31 13:12 PST, sugoi
no flags Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2013-02-13 14:13 PST, sugoi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sugoi 2013-01-23 09:46:44 PST
This bug was reported in Chromium as Issue 169024, but it was reproduced in other browsers, so it's not a Chromium specific bug.

Go to the provided URL and drag the green square using the mouse. The red square will lag behind even though it should be in sync with the green square.
Removing the line "overflow: hidden;" from the "overlay" element solves the issue, so the "overflow: hidden;" line triggers this issue somehow.
Comment 1 sugoi 2013-01-23 09:53:02 PST
Created attachment 184256 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-23 10:48:25 PST
Comment on attachment 184256 [details]
Patch

Attachment 184256 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16073324

New failing tests:
svg/custom/text-xy-updates-SVGList.xhtml
svg/dynamic-updates/SVGUseElement-svgdom-href1-prop.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
svg/custom/circle-move-invalidation.svg
svg/custom/mask-invalidation.svg
fast/repaint/search-field-cancel.html
fast/repaint/moving-shadow-on-path.html
svg/transforms/animated-path-inside-transformed-html.xhtml
fast/repaint/inline-relative-positioned.html
svg/repaint/repaint-webkit-svg-shadow-container.html
Comment 3 sugoi 2013-01-23 11:08:41 PST
I don't know whether or not it's the correct fix, but it does fix the issue. So, basically, here's the problem I found :

There are 2 different codepaths to doing a relayout :
1 ) If you have overflow:visible, then you may write anywhere in your parent's canvas, so your parent has to relayout too.
- Doing a relayout all the way up will trigger a relayout of the RenderView object and this will work because everything gets to have a relayout. So here, FrameView::scheduleRelayout() is called from RenderObject::scheduleRelayout(), which was called from RenderObject::markContainingBlocksForLayout().

2 ) In any other overflow mode, you clip your content to your own canvas, so no need for your parent to relayout, so we stop there. 
- So since we stop early in RenderObject::markContainingBlocksForLayout(), when we get to RenderObject::scheduleRelayout(), we'll actually call FrameView::scheduleRelayoutOfSubtree(). The fix I wrote is inside that function. It seems it wasn't recursively marking contained objects for relayout when m_layoutSchedulingEnabled is true, for some reason.

If the fix isn't the appropriate fix, please explain how the m_layoutSchedulingEnabled logic should work.

Thanks
Comment 4 Dimitri Glazkov (Google) 2013-01-23 12:50:35 PST
Comment on attachment 184256 [details]
Patch

One of the things that might be great is your fiddle converted into a layout test.

Also, the fix looks wrong -- you're simply invalidating "the whole thing". But I defer to paint experts to guide you here :)
Comment 5 sugoi 2013-01-23 13:00:53 PST
(In reply to comment #4)
> (From update of attachment 184256 [details])
> One of the things that might be great is your fiddle converted into a layout test.

I have a layout test locally, I'll add it as soon as I know that it validates the "proper" fix.

> Also, the fix looks wrong -- you're simply invalidating "the whole thing". But I defer to paint experts to guide you here :)

I'm not sure I follow you about invalidating "the whole thing". The path where m_layoutSchedulingEnabled is true is the only path not calling "markContainingBlocksForLayout". Does that mean that when m_layoutSchedulingEnabled is false, we always invalidate "the whole thing" ?

Also, at the point where this function is called, m_layoutRoot points to the current object (in the reference URL, it would be pointing to the bottom right canvas of the page), which is the root of the subtree and we do want to invalidate the subtree, I believe, since we're about to render it.
Comment 6 James Robinson 2013-01-23 16:44:03 PST
Comment on attachment 184256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184256&action=review

> Source/WebCore/page/FrameView.cpp:2258
> +        m_layoutRoot->markContainingBlocksForLayout(false);

this basically disables the subtree relayout optimization.

The idea of the subtree layout optimization is it's applied when the only layout we need to do is within the subtree.  In your test case, likely one of the following is true:

1.) The subtree is incorrectly rooted (too low in the tree)
2.) The subtree optimization shouldn't be used in this instance
3.) The subtree optimization is correct, but something else is awry

I would wager it's 3.  This call will cover up many sins by causing us to relayout and reinvalidation pretty much the entire world every frame.

I think you should try to back out to the narrowest point that's misbehaving.  Perhaps something isn't marked as needing layout that should, or perhaps everything is being laid out correctly but something isn't generating correct invalidations (aka repaints in the RenderObject world)
Comment 7 sugoi 2013-01-28 14:17:56 PST
Ok, at this point, I'm stuck, I need the input of someone more knowledgeable than me on the compositing tree.

It seems that the compositing tree is incompatible with partial (subtree) layout updates. Here's why :

When we call WebCore::RenderLayerModelObject::styleDidChange(), here's what happens :

1)
WebCore::RenderLayerModelObject::styleDidChange
WebCore::RenderObject::styleDidChange
WebCore::RenderObject::setNeedsPositionedMovementLayout
WebCore::RenderObject::markContainingBlocksForLayout
WebCore::RenderObject::scheduleRelayout 
WebCore::FrameView::scheduleRelayoutOfSubtree

2)
WebCore::RenderLayerModelObject::styleDidChange (same call as stack trace 1)
WebCore::RenderLayer::styleChanged
WebCore::RenderLayerCompositor::updateLayerCompositingState
WebCore::RenderLayerBacking::updateGraphicsLayerConfiguration
...
-> Eventually, we read a position value here and set the layer position

Unfortunately, the layer position is not updated yet. It will be in the next FrameView::layout() call when, after updating the position value by calling layout on the subtree root, we'll call "updateLayerPositionsAfterLayout()". That call has to happen before the we call updateLayerCompositingState(), otherwise, the layer position will be wrong.

I also get a weird behavior when I'm debugging a page that shows this bug. When I refresh it multiple times, I get different results, as if the timers were sometimes getting triggered in a different order.

I don't want to implement a solution where I'd call layout(), then updateLayerCompositingState(), then layout(), then updateLayerCompositingState() again, since that would be obviously sub-optimal.

I'll wait for someone to chime in and give me some sort of explanation about the expected relationship between the layout operation and the compositing tree before I continue my investigation.
Comment 8 sugoi 2013-01-29 11:12:08 PST
Created attachment 185271 [details]
Patch
Comment 9 sugoi 2013-01-29 11:24:26 PST
Ok, I tried this new patch to fix the issue. The original fix was breaking the subtree optimization, as James pointed out, so I tried to find something less aggressive, but the current fix may still be too much for what we need, I'll let the reviewers decide.

In this version of the fix, only accelerated compositing is affected and it is only affected when a layout call using the subtree optimization causes a layer to move. In that specific case, the position of all layers gets updated so that we don't miss any update before computing the compositing tree. It is slightly redundant, because the subtree will get its position updated twice, potentially, but it's probably better than to update the whole tree more frequently since we use the result of the 1st call to updateLayerPositionsAfterLayout() in order to know if a top layer call is necessary.
Comment 10 Simon Fraser (smfr) 2013-01-29 11:25:52 PST
(In reply to comment #7)
> Ok, at this point, I'm stuck, I need the input of someone more knowledgeable than me on the compositing tree.
> 
> It seems that the compositing tree is incompatible with partial (subtree) layout updates. Here's why :
> 
> When we call WebCore::RenderLayerModelObject::styleDidChange(), here's what happens :
> 
> 1)
> WebCore::RenderLayerModelObject::styleDidChange
> WebCore::RenderObject::styleDidChange
> WebCore::RenderObject::setNeedsPositionedMovementLayout
> WebCore::RenderObject::markContainingBlocksForLayout
> WebCore::RenderObject::scheduleRelayout 
> WebCore::FrameView::scheduleRelayoutOfSubtree
> 
> 2)
> WebCore::RenderLayerModelObject::styleDidChange (same call as stack trace 1)
> WebCore::RenderLayer::styleChanged
> WebCore::RenderLayerCompositor::updateLayerCompositingState
> WebCore::RenderLayerBacking::updateGraphicsLayerConfiguration
> ...
> -> Eventually, we read a position value here and set the layer position

This part sounds wrong; if we're inside styleChanged (and there's a pending layout), then we can't rely on any geometry information (since layout might change it). The code does do this right now, but what should happen is a subsequent compositing layer update after the next layout should fix it.  Is that happening?
Comment 11 Simon Fraser (smfr) 2013-01-29 11:27:28 PST
Comment on attachment 185271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185271&action=review

> Source/WebCore/page/FrameView.cpp:1256
> +    if (subtree && positionChanged) {
> +        RenderObject* topRoot = document->renderer();
> +        if (topRoot) {
> +            RenderLayer* topLayer = topRoot->enclosingLayer();
> +            topLayer->updateLayerPositionsAfterLayout(rootRenderer(this)->layer(),
> +                updateLayerPositionFlags(topLayer, false, m_doFullRepaint));
> +        }
> +    }

This doesnt' seem compositing-related. We rely on RenderLayer positions for invalidation etc too.
Comment 12 Simon Fraser (smfr) 2013-01-29 11:44:56 PST
Ah, the bug here is that
  layer = root->enclosingLayer();
in FrameView::layout() doesn't go high enough up the tree in this case; it needs to find the enclosing stacking container.
Comment 13 Simon Fraser (smfr) 2013-01-29 12:12:00 PST
So that should be

layer = root->enclosingLayer()-> enclosingStackingContainer();
Comment 14 sugoi 2013-01-30 08:27:55 PST
Created attachment 185509 [details]
Patch
Comment 15 sugoi 2013-01-30 08:34:45 PST
I tried to use Simon's solution, but it did not fix the issue. In the cases I've tested, unless we start at the document's root's layer, the bug is still present. The proposed solution works in the cases I've tested with and, unless calling updateLayerPositionsAfterLayout() on the top layer is a terribly time consuming operation, I think/hope this fix could be acceptable, because I currently have no alternative solution :) I'll investigate more if this doesn't work for you.
Comment 16 Build Bot 2013-01-30 09:12:33 PST
Comment on attachment 185509 [details]
Patch

Attachment 185509 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16218587

New failing tests:
fast/canvas/canvas-overflow-hidden-animation.html
Comment 17 Simon Fraser (smfr) 2013-01-30 09:19:21 PST
Comment on attachment 185509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185509&action=review

> Source/WebCore/page/FrameView.cpp:1197
> -        layer = root->enclosingLayer();
> +        layer = document->renderer()->enclosingLayer();

This is wrong.

You want layer = root->enclosingLayer()->enclosingStackingContainer().

> LayoutTests/ChangeLog:11
> +        * fast/canvas/canvas-overflow-hidden-animation.html: Added.

This test is not automatic or self-testing. It needs to detect the incorrect condition without the patch (either with some self-check, or by comparing against a reference).
Comment 18 sugoi 2013-01-30 09:37:36 PST
Comment on attachment 185509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185509&action=review

>> Source/WebCore/page/FrameView.cpp:1197
>> +        layer = document->renderer()->enclosingLayer();
> 
> This is wrong.
> 
> You want layer = root->enclosingLayer()->enclosingStackingContainer().

I agree that this is not what we discussed yesterday, but as I mentioned in the comments, "layer = root->enclosingLayer()->enclosingStackingContainer()" does not fix the issue.
Comment 19 Simon Fraser (smfr) 2013-01-30 09:42:45 PST
Hm, interesting. We should figure out why it does not work.
Comment 20 Simon Fraser (smfr) 2013-01-30 10:55:37 PST
(In reply to comment #15)
> I tried to use Simon's solution, but it did not fix the issue. In the cases I've tested, unless we start at the document's root's layer, the bug is still present. The proposed solution works in the cases I've tested with and, unless calling updateLayerPositionsAfterLayout() on the top layer is a terribly time consuming operation, I think/hope this fix could be acceptable, because I currently have no alternative solution :) I'll investigate more if this doesn't work for you.

You should use showLayerTree() in the debugger to see what's different between the layer tree between an update from the root, and an update from the enclosingStackingContainer.
Comment 21 sugoi 2013-01-30 11:25:36 PST
Using showLayerTree() shows that both trees (the good one and the bad one) are identical after calling updateCompositingLayersAfterLayout(). There must be something (like a redraw or an invalidation) triggered only by calling updateCompositingLayersAfterLayout() on the top layer.
Comment 22 sugoi 2013-01-30 13:34:45 PST
Created attachment 185551 [details]
Patch
Comment 23 sugoi 2013-01-30 13:39:02 PST
So I have a new proposal for the fix. I was expecting this new code, or something equivalent, to be done within RenderLayerCompositor::updateCompositingLayers() somewhere, but it seems like there's no guarantee of that happening, so I added it directly in RenderLayer::updateLayerPositionsAfterLayout() since similar code is executed within RenderLayer::updateLayerPositions()
Comment 24 Simon Fraser (smfr) 2013-01-30 13:49:44 PST
Comment on attachment 185551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185551&action=review

> Source/WebCore/rendering/RenderLayer.cpp:386
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((flags & UpdateCompositingLayers) && rootLayer && (rootLayer != this) && rootLayer->isComposited()) {
> +        RenderLayerBacking::UpdateAfterLayoutFlags updateFlags = RenderLayerBacking::CompositingChildrenOnly;
> +        if (flags & NeedsFullRepaintInBacking)
> +            updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> +        if (flags & IsCompositingUpdateRoot)
> +            updateFlags |= RenderLayerBacking::IsUpdateRoot;
> +        rootLayer->backing()->updateAfterLayout(updateFlags);
> +    }

What does this do that the later updateCompositingLayersAfterLayout() call in FrameView::layout() does not?

Your changelog needs to convince me that you understand the underlying cause for this bug. This feels like wallpapering over the cracks.
Comment 25 sugoi 2013-01-31 06:22:36 PST
Created attachment 185769 [details]
Patch
Comment 26 sugoi 2013-01-31 06:28:01 PST
Comment on attachment 185769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185769&action=review

> Source/WebCore/rendering/RenderLayer.cpp:380
> +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {

@smfr: You were right, the enclosingCompositingLayer() function returns the proper layer, no need to use the root layer directly. They were probably the same in my test cases, which is why these cases required the root layer (also, I didn't know about the enclosingCompositingLayer() function), but this code also fixes the issue.
Comment 27 Build Bot 2013-01-31 08:11:16 PST
Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16251416

New failing tests:
fast/canvas/canvas-overflow-hidden-animation.html
Comment 28 Simon Fraser (smfr) 2013-01-31 09:12:16 PST
Comment on attachment 185769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185769&action=review

> Source/WebCore/rendering/RenderLayer.cpp:390
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((flags & UpdateCompositingLayers) && (rootLayer != this)) {
> +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {
> +            RenderLayerBacking::UpdateAfterLayoutFlags updateFlags =
> +                RenderLayerBacking::CompositingChildrenOnly;
> +            if (flags & NeedsFullRepaintInBacking)
> +                updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> +            if (flags & IsCompositingUpdateRoot)
> +                updateFlags |= RenderLayerBacking::IsUpdateRoot;
> +            compLayer->backing()->updateAfterLayout(updateFlags);
> +        }
> +    }
> +#endif

Does it not work just to start the updateLayerPositions() at the enclosingCompositingLayer()? I think add new updateAfterLayout() calls here is going to result in extra work in many more cases.
Comment 29 sugoi 2013-01-31 10:06:16 PST
(In reply to comment #28)
> (From update of attachment 185769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185769&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:390
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    if ((flags & UpdateCompositingLayers) && (rootLayer != this)) {
> > +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {
> > +            RenderLayerBacking::UpdateAfterLayoutFlags updateFlags =
> > +                RenderLayerBacking::CompositingChildrenOnly;
> > +            if (flags & NeedsFullRepaintInBacking)
> > +                updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> > +            if (flags & IsCompositingUpdateRoot)
> > +                updateFlags |= RenderLayerBacking::IsUpdateRoot;
> > +            compLayer->backing()->updateAfterLayout(updateFlags);
> > +        }
> > +    }
> > +#endif
> 
> Does it not work just to start the updateLayerPositions() at the enclosingCompositingLayer()? I think add new updateAfterLayout() calls here is going to result in extra work in many more cases.

Sure, it should work, but since this added code is called within updateLayerPositions(), around line 463, I thought it would be less costly to just call these lines rather than the whole function. I can easily change it to call the whole function, but I don't fully understand how that would affect performance positively.
Comment 30 WebKit Review Bot 2013-01-31 11:24:50 PST
Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16273326

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-overflow-hidden-animation.html
Comment 31 sugoi 2013-01-31 11:52:41 PST
I redid my tests after seeing the failures and I made a mistake while doing my last tests. The current patch (using enclosingCompositingLayer()) doesn't work. Only the 4th patch, using the root layer, works, so it seems I'm back to patch number 4 as the latest working solution.
Comment 32 WebKit Review Bot 2013-01-31 12:05:14 PST
Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16269354

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-overflow-hidden-animation.html
Comment 33 sugoi 2013-01-31 13:12:42 PST
Created attachment 185846 [details]
Patch
Comment 34 sugoi 2013-01-31 13:18:11 PST
This new patch only does the missing geometry update for the current compositor, only for composited children and nothing else, so it may be a bit more efficient then the previous patch, and this also fixes the issue.
Comment 35 sugoi 2013-02-13 14:13:40 PST
Created attachment 188179 [details]
Patch
Comment 36 sugoi 2013-02-13 14:22:01 PST
I found a way to fix the bug using the enclosingStackingContainer().

The issue was that, even if we would use the enclosingStackingContainer() as the root of the update inside RenderLayer, if that container isn't itself composited, it wouldn't trigger any updates in the accelerated compositing section, which is why the original solution using enclosingStackingContainer() failed.

We do need to update from the enclosingStackingContainer() because only a stacking container can update the Z Order lists inside RenderLayerCompositor::updateCompositingDescendantGeometry() and these need to be updated, even on a subtree relayout.

The current fix should hopefully be less costly than the original proposition, but still do the proper updates.
Comment 37 Simon Fraser (smfr) 2013-02-21 14:04:10 PST
Comment on attachment 188179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188179&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:465
>          if (flags & IsUpdateRoot) {
>              updateGraphicsLayerGeometry();
>              layerCompositor->updateRootLayerPosition();
> +            RenderLayer* stackingContainer = m_owningLayer->enclosingStackingContainer();
> +            if (!layerCompositor->compositingLayersNeedRebuild() && stackingContainer && (stackingContainer != m_owningLayer))
> +                layerCompositor->updateCompositingDescendantGeometry(stackingContainer, stackingContainer, flags & CompositingChildrenOnly);

So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?
Comment 38 sugoi 2013-02-21 14:08:11 PST
> So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?

I don't think so, it won't do anything if stackingContainer == m_owningLayer, so if m_owningLayer is root, then its stacking container should either be itself or NULL (I don't know if NULL is possible for the root layer).
Comment 39 sugoi 2013-02-21 14:11:36 PST
(In reply to comment #38)
> > So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?
> 
> I don't think so, it won't do anything if stackingContainer == m_owningLayer, so if m_owningLayer is root, then its stacking container should either be itself or NULL (I don't know if NULL is possible for the root layer).

But if you meant root as in m_owningLayer which is not the root layer, then yes, what you said is correct. Note that this is already the case as this function is called recursively from leaves to root, so updateCompositingDescendantGeometry() is already called in that fashion (children first, parents afterwards).
Comment 40 sugoi 2013-02-21 14:14:19 PST
What I'm describing is from the line :

backing()->updateAfterLayout(updateFlags);

(approximately line 464 in file RenderLayer.cpp)
Comment 41 sugoi 2013-03-04 13:13:50 PST
Friendly ping to reviewers. There's been no activity on this cl for more than a week, now. It's only 3 lines of code :)
Comment 42 Eric Seidel (no email) 2013-03-04 13:16:06 PST
jamesr or smfr are your best reviewers.  I would need someone to bring me up to speed on current RenderLayer thinking before reviewing.
Comment 43 Simon Fraser (smfr) 2013-03-04 13:47:28 PST
Comment on attachment 188179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188179&action=review

>> Source/WebCore/rendering/RenderLayerBacking.cpp:465
>> +                layerCompositor->updateCompositingDescendantGeometry(stackingContainer, stackingContainer, flags & CompositingChildrenOnly);
> 
> So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?

Actually I think this is fine.
Comment 44 WebKit Review Bot 2013-03-04 14:18:48 PST
Comment on attachment 188179 [details]
Patch

Clearing flags on attachment: 188179

Committed r144674: <http://trac.webkit.org/changeset/144674>
Comment 45 WebKit Review Bot 2013-03-04 14:18:55 PST
All reviewed patches have been landed.  Closing bug.