1021564 - [toolbox] Toggling toolbox buttons acts weirdly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Patrick Brosset <:pbro>]

Not sure how to phrase this bug's summary better, but this video explains it all:

https://www.youtube.com/watch?v=0kTRuEMcNPs

While disabling toolbox buttons, at some stage, 2 weird things happens:
- disabling the responsive mode button removes the button but inserts a new paintflashing button
- going on and removing more buttons: the paintflashing button stays visible in the middle of the toolbar and only goes away when the window it is resized.

I managed to reproduce this problem locally on nightly.
The order in which you disable buttons seems to be important.
Have them all selected, and then start unselecting from the top (pick an element, split console, responsive mode, ...).

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #0)
> Not sure how to phrase this bug's summary better, but this video explains it
> all:
> 
> https://www.youtube.com/watch?v=0kTRuEMcNPs
> 
> While disabling toolbox buttons, at some stage, 2 weird things happens:
> - disabling the responsive mode button removes the button but inserts a new
> paintflashing button
> - going on and removing more buttons: the paintflashing button stays visible
> in the middle of the toolbar and only goes away when the window it is
> resized.
> 
> I managed to reproduce this problem locally on nightly.
> The order in which you disable buttons seems to be important.
> Have them all selected, and then start unselecting from the top (pick an
> element, split console, responsive mode, ...).

I noticed this when we started having the display glitches in Nightly.  The weird thing is that if you resize the toolbox at any stage things seems to get back in sync.  I can check with browser toolbox to confirm that buttons are actually being removed

Comment 2

[Patrick Brosset <:pbro>]

Randomly removing/adding buttons, I managed to get to this stage:
https://dl.dropboxusercontent.com/u/714210/Screenshot%202014-06-06%2014.45.25.png

Also worth noting that the problem goes away as soon as you switch to another app too, no need to resize the window.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> Randomly removing/adding buttons, I managed to get to this stage:
> https://dl.dropboxusercontent.com/u/714210/Screenshot%202014-06-06%2014.45.
> 25.png
> 
> Also worth noting that the problem goes away as soon as you switch to
> another app too, no need to resize the window.

I can see that too, but only if the toolbox is at a certain resolution.  If I make the window more narrow the problem seems to go away.

Comment 4

[Paul Rouget [:paul]]

Does it still happen if you switch layers.offmainthreadcomposition.enabled to false?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> Does it still happen if you switch layers.offmainthreadcomposition.enabled
> to false?

Yes, still happens after setting that pref and restarting

Comment 6

[obrufau]

Good: cbf99fb64ee9 (1399923357 inbound)
Bad: 06800eda477b (1399923539 inbound)
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cbf99fb64ee9&tochange=06800eda477b

So this is caused by Bug 1008301 or Bug 1008578.

Comment 7

[obrufau]

I can reproduce it on Windows XP, so the Operating System field should be changed to All (I don't have enough privileges to change it).

Comment 8

[Tim Nguyen :ntim]

Seems like only the light theme is affected, couldn't reproduce on dark theme.
Maybe it's related to the fact that we use CSS Filters.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #8)
> Seems like only the light theme is affected, couldn't reproduce on dark
> theme.
> Maybe it's related to the fact that we use CSS Filters.

Weird, it only seems to be an issue on the light theme for me too.

Comment 10

[Brian Grinstead [:bgrins]]

I think that removing command buttons from the big list of filter elements does actually remove the glitch: https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#783.  Of course, you can't see any of the images anymore because they are white, but this helps narrow it down.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: hack-translate.patch

This actually fixes the problem for me, but I don't know about landing it this way

Comment 12

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Created attachment 8437971 [details] [diff] [review]
> hack-translate.patch
> 
> This actually fixes the problem for me, but I don't know about landing it
> this way

I think there's a will-change property implemented in Firefox (bug 940842). It's an alternative to the transform hack. Although, I've never tested it.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: filters-bug.zip

I see the same issue in web content when pressing the 'hide1' button here

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Created attachment 8437989 [details]
> filters-bug.zip
> 
> I see the same issue in web content when pressing the 'hide1' button here

Markus, does this issue seem like it could have been caused by Bug 1008301 or Bug 1008578?  These were the bugs tracked down in Comment 6.

Comment 15

[Markus Stange [:mstange]]

Definitely.

Comment 16

[Markus Stange [:mstange]]

Attachment: testcase 1

Hovering the document should make the 100x100 green square move and not leave any green stuff behind.

Comment 17

[Markus Stange [:mstange]]

Attachment: testcase 2

should behave the same as testcase 1

Comment 18

[Brian Grinstead [:bgrins]]

This is affecting 32. The problem is seen on https://bug1021564.bugzilla.mozilla.org/attachment.cgi?id=8439797 in the latest Aurora 32.0a2 (2014-07-15)

Comment 19

[Markus Stange [:mstange]]

Sorry, I should have added a comment with that change.
This is fixed on Aurora since yesterday because I backed out bug 1008301 from Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0fcb1dc73d58

Comment 20

[Markus Stange [:mstange]]

Attachment: v1

Comment 21

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

Comment on attachment 8456866 [details] [diff] [review]
v1

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

::: layout/base/nsDisplayListInvalidation.h
@@ +135,5 @@
> +class nsDisplaySVGEffectsGeometry : public nsDisplayItemGeometry
> +{
> +public:
> +  nsDisplaySVGEffectsGeometry(nsDisplaySVGEffects* aItem, nsDisplayListBuilder* aBuilder);
> +  

trailing whitespace

Comment 22

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ae841e4f92

Comment 23

[Carsten Book [:Tomcat]]

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=44026850&tree=Mozilla-Inbound&full=1 

06:49:04     INFO -  [965] ###!!! ASSERTION: How did we getting here, then?: 'aFrame->GetParent()->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozAnonymousBlock', file /builds/slave/m-in-osx64-d-00000000000000000/build/layout/svg/nsSVGIntegrationUtils.cpp, line 111
06:49:04     INFO -  PreEffectsVisualOverflowCollector::AddBox(nsIFrame*) [layout/svg/nsSVGIntegrationUtils.cpp:62]
06:49:04     INFO -  nsLayoutUtils::GetAllInFlowBoxes(nsIFrame*, nsLayoutUtils::BoxCallback*) [layout/base/nsLayoutUtils.cpp:3413]
06:49:04     INFO -  GetPreEffectsVisualOverflowUnion [obj-firefox/dist/include/nsRect.h:39]
06:49:04     INFO -  nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame*) [obj-firefox/dist/include/nsRect.h:54]
06:49:04     INFO -  nsSVGUtils::GetBBox(nsIFrame*, unsigned int) [layout/svg/nsSVGUtils.cpp:984]
06:49:04     INFO -  nsDisplaySVGEffectsGeometry::nsDisplaySVGEffectsGeometry(nsDisplaySVGEffects*, nsDisplayListBuilder*) [layout/base/nsDisplayList.cpp:5262]
06:49:04     INFO -  nsDisplaySVGEffects::AllocateGeometry(nsDisplayListBuilder*) [layout/base/nsDisplayList.h:3141]
06:49:04     INFO -  mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*, unsigned int) [layout/base/FrameLayerBuilder.cpp:2922]
06:49:04     INFO -  mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) [layout/base/FrameLayerBuilder.cpp:603]
06:49:04     INFO -  nsDisplayTransform::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) [obj-firefox/dist/include/nsCOMPtr.h:175]
06:49:04     INFO -  mozilla::FrameLayerBuilder::AddThebesDisplayItem(mozilla::ThebesLayerData*, nsDisplayItem*, mozilla::DisplayItemClip const&, nsIntRect const&, mozilla::ContainerState const&, mozilla::LayerState, nsPoint const&, nsAutoPtr<nsDisplayItemGeometry>) [obj-firefox/dist/include/nsCOMPtr.h:175]
06:49:04     INFO -  mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*, unsigned int) [obj-firefox/dist/include/nsAutoPtr.h:73]
06:49:04     INFO -  mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) [layout/base/FrameLayerBuilder.cpp:603]
06:49:04     INFO -  nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) [obj-firefox/dist/include/nsCOMPtr.h:175]
06:49:04     INFO -  nsDisplaySubDocument::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) [obj-firefox/dist/include/nsCOMPtr.h:175]
06:49:04     INFO -  mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*, unsigned int) [obj-firefox/dist/include/nsCOMPtr.h:175]
06:49:04     INFO -  mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) [layout/base/FrameLayerBuilder.cpp:603]
06:49:04     INFO -  nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) [layout/base/nsIPresShell.h:1328]
06:49:04     INFO -  nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) [obj-firefox/dist/include/GeckoProfilerImpl.h:370]
06:49:04     INFO -  PresShell::Paint(nsView*, nsRegion const&, unsigned int) [layout/base/nsPresShell.cpp:6222]
06:49:04     INFO -  nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) [obj-firefox/dist/include/nsRegion.h:55]
06:49:04     INFO -  nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) [view/src/nsViewManager.cpp:380]
06:49:04     INFO -  nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [layout/base/nsLayoutUtils.h:2067]
06:49:04     INFO -  mozilla::RefreshDriverTimer::Tick() [obj-firefox/dist/include/nsTArray.h:855]
06:49:04     INFO -  nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:634]
06:49:04     INFO -  nsTimerEvent::Run() [obj-firefox/dist/include/nsAutoPtr.h:836]
06:49:04     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:766]
06:49:04     INFO -  NS_ProcessPendingEvents(nsIThread*, unsigned int) [xpcom/glue/nsThreadUtils.cpp:207]
06:49:04     INFO -  nsBaseAppShell::NativeEventCallback() [widget/xpwidgets/nsBaseAppShell.cpp:99]
06:49:04     INFO -  nsAppShell::ProcessGeckoEvents(void*) [widget/cocoa/nsAppShell.mm:375]
06:49:04     INFO -  CoreFoundation + 0x4e401
06:49:04     INFO -  CoreFoundation + 0x4c5f9
06:49:04     INFO -  CoreFoundation + 0x4bdbf
06:49:04     INFO -  HIToolbox + 0x2e7ee
06:49:04     INFO -  HIToolbox + 0x2e5f3
06:49:04     INFO -  HIToolbox + 0x2e4ac
06:49:04     INFO -  AppKit + 0x43eb2
06:49:04     INFO -  -AppKit + 0x43801
06:49:04     INFO -  -[G

Comment 25

[Markus Stange [:mstange]]

I backed out bug 1008301 (which caused this) from Aurora again so that this bug is fixed on 33.

[Tracking Requested - why for this release]: Requesting tracking to be moved from 33 to 34.

Hopefully I'll be able to finish this up today or tomorrow.

Comment 26

[Tim Nguyen :ntim]

Attachment: css-filters-testcase.html

A testcase with CSS Filters. The bug is reproducible too.

Comment 27

[Tim Nguyen :ntim]

Attachment: css-filters-testcase-reference.html

This is how attachment 8477372 [details] should look like.

Comment 29

[Markus Stange [:mstange]]

Attachment: mathml fix

The testcase that the assertion from comment 23 fires on (473278-1.xhtml) looks like this:

<html xmlns="http://www.w3.org/1999/xhtml"><head></head><body><mmultiscripts xmlns="http://www.w3.org/1998/Math/MathML" style="clip-path: url(#q); -moz-transform: translate(100px, 100px);"/></body></html>

and has this frame tree:

> Viewport(-1)@12945d450 [view=12937df20] {0,0,48000,60000} [state=0001062000002220] [sc=129a2dea8:-moz-viewport]<
>   HTMLScroll(html)(-1)@12945e190 {0,0,48000,60000} [state=0000020000000010] [content=129458480] [sc=129a2e9d0:-moz-viewport-scroll]<
>     ScrollbarFrame(scrollbar)(-1)@12945e978 next=1294ebea0 {0,60000,48000,0} [state=0000100080c80000] [content=129428a10] [sc=129a2ef50]<
>       SliderFrame(slider)(-1)@1294eb520 {0,0,48000,960} [state=0000160080c00000] [content=129428d70] [sc=12945edf8]<
>         ButtonBoxFrame(thumb)(0)@1294eb9e8 {60,120,47880,780} [state=2000160080400000] [content=129428e00] [sc=1294ebfa0]<>
>       >
>     >
>     ScrollbarFrame(scrollbar)(-1)@1294ebea0 next=129a2d2b0 {48000,0,0,60000} [state=0000100080880000] [content=129428aa0] [sc=1294eb750]<
>       SliderFrame(slider)(-1)@1294ec7f8 {0,0,960,60000} [state=0000160080800000] [content=129429280] [sc=1294ec280]<
>         ButtonBoxFrame(thumb)(0)@1294ecc30 {120,60,780,59880} [state=2000160080000000] [content=129429310] [sc=129a2ddf0]<>
>       >
>     >
>     Box(scrollcorner)(-1)@129a2d2b0 next=12945de80 {48000,60000,0,0} [state=0000100080c00200] [content=129428b30] [sc=1294ec998]<>
>     Canvas(html)(-1)@12945de80 {0,0,48000,60000} [state=0000002000000200] [content=129458480] [sc=1294ece28:-moz-scrolled-canvas]<
>       Block(html)(-1)@129a2d910 {0,0,48000,2112} [state=0000100000d00200] [content=129458480] [sc=129a2d470,parent=0]<
>         line 129a2dfe8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x48] bm=480 {480,480,47040,1152} <
>           Block(body)(1)@129a2df50 {480,480,47040,1152} [state=0000120000100200] [content=129485520] [sc=129a2dac0]<
>             line 129a2e328: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,5914,1152} <
>               Frame(mmultiscripts)(0)@129a2e260 {0,96,5914,960} [state=0000100000010000] transformed [content=1294586c0] [sc=12945d0f8]<>
>             >
>           >
>         >
>       >
>     >
>   >
> >

The mmultiscripts frame is in error and shows "invalid-markup".

We call nsSVGUtils::GetBBox on the mmultiscripts frame and assert because that frame doesn't have a PreEffectsBBoxProperty. The PreEffectsBBoxProperty would be set in ComputeEffectsRect during FinishAndStoreOverflow, but the mmultiscripts frame never calls FinishAndStoreOverflow because nsMathMLContainerFrame::FinalizeReflow, which would usually call FinishAndStoreOverflow through GatherAndStoreOverflow, returns early due to the error state.

Can we just call GatherAndStoreOverflow here regardless? Or is there a better fix?

https://tbpl.mozilla.org/?tree=Try&rev=fb7ea1e10c8a

Comment 30

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

Comment on attachment 8481781 [details] [diff] [review]
mathml fix

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

looks good to me

Comment 31

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0d8ef32b3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c0f68531d4b

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c0d8ef32b3e
https://hg.mozilla.org/mozilla-central/rev/2c0f68531d4b

Comment 33

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced issue with steps from comment 0 and also testcases from comment 16 and comment 17, using Nightly from August 29 (BuildID=20140829030204), on Windows 7 x64, Mac OS X 10.9.4, Ubuntu 13.04 x64.

No issues were seen on Windows 7 x64, Mac OS X 10.9.4, Ubuntu 13.04 x64, using:
- Firefox 32 Release Candidate - BuildID=20140825202822
- Latest Firefox 33 Aurora - BuildID=20140831004002
- Latest Firefox 34 Nightly - BuildID=20140831030206