807457 - [Browser] SVG content not repainted, when coming back from tab listing, or navigating back to it w/ "back" button

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load testcase in B2G browser
 2. Click the "1>" in upper-right corner to open tab-view
 3. Go back to your tab*

EXPECTED RESULTS: The tab should look like it did after step 1.

ACTUAL RESULTS: The tab is blank.


* It doesn't seem to matter *how* you get back to your tab -- I tried tapping its entry in the list, or tapping the upper-left "<" button, or tapping the left edge of the screen -- and I hit the bug in each case.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (lime rect 100% by 100%)

Here's a fully-lime testcase, taken from our source code:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/lime.svg?raw=1&ctype=image/svg+xml

Comment 2

[Daniel Holbert [:dholbert]]

I also hit this bug if I...
 (1) view the testcase
 (2) navigate to a different site (e.g. http://pastebin.mozilla.org)
 (3) hit "back" button

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

related to bug 806035?

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Naoki Hirata :nhirata from comment #3)
> related to bug 806035?

Hard to say.  That bug has blurriness, whereas here, it's just blank white nothingness. And I can't reproduce that bug (though I have seen random blurriness from time to time).

Comment 5

[Daniel Holbert [:dholbert]]

...though I can reliably reproduce bug 806035 by going to the tab listing & back (per bug 806035 comment 2)

Given that, these bugs are likely related. (though note that bug 806035 requires zooming whereas this bug here does not)

Comment 6

[Andrew Overholt [:overholt]]

roc, any ideas about what could be causing this?

Comment 7

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

No. Are we hitting _cairo_error?

Comment 8

[JP Rosevear [:jpr]]

Daniel, can you answer #7?

Comment 9

[Daniel Holbert [:dholbert]]

I can try. Haven't installed a custom / debug b2g build on my dogfood device yet, so i imagine there'll be a bit of spin-up before I can answer, but I'll see what I can find out.

FWIW, I tried the desktop FxOS simulator and was unable to reproduce there (using steps in comment 0 & comment 2), so this appears to be specific to B2G-running-on-a-device.

Comment 10

[Daniel Holbert [:dholbert]]

In reply to comment 7 / comment 8: We're definitely not hitting _cairo_error. (added some android logging there, and it's not getting hit)

I added some logging to various BuildDisplayList methods, and from comparing another testcase w/ an HTML <table> element,  I got these results:
 - initial load: nsTableFrame::BuildDisplayList is called
 - return from tabstrip: nsTableFrame::BuildDisplayList is called
VS. for the attached SVG testcase...
 - initial load: nsSVGOuterSVGFrame::BuildDisplayList is called
 - return from tabstrip: nsSVGOuterSVGFrame::BuildDisplayList is NOT CALLED

Comment 11

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

Try logging with DEBUG_INVALIDATIONS?

Comment 12

[Daniel Holbert [:dholbert]]

That confirms that we're invalidating/painting a lot less, but I haven't decoded exactly why yet.

One more data point: comparing a pure-SVG testcase vs. SVG-in-HTML5 testcase -- when we return from the tabstrip, the pure-SVG gets a call to ViewportFrame::BuildDisplayList with an empty dirtyrect (0,0,0,0), whereas the SVG-in-HTML5 testcase gets a call with a huge dirtyrect (0,0,58800,67020).

Comment 13

[Daniel Holbert [:dholbert]]

This traces back to differing calls to nsDOMWindowUtils::SetDisplayPortForElement().
- In the HTML case, I get a call with aXPx=0, aYPx=0, aWidthPx=980, aHeightPx=1117
- In the SVG case, I get a call with aXPx=0, aYPx=0, aWidthPx=0, aHeightPx=0

Comment 14

[Daniel Holbert [:dholbert]]

OK, so this basically boils down to us assuming we're viewing an HTML document.

This traces back to this chunk of code, in TabChild.cpp:
> 411   nsCOMPtr<nsIDOMElement> htmlDOMElement = do_QueryInterface(document->GetHtmlElement());
> 412   nsCOMPtr<nsIDOMElement> bodyDOMElement = do_QueryInterface(document->GetBodyElement());
> 413 
> 414   int32_t htmlWidth = 0, htmlHeight = 0;
> 415   if (htmlDOMElement) {
> 416     htmlDOMElement->GetScrollWidth(&htmlWidth);
> 417     htmlDOMElement->GetScrollHeight(&htmlHeight);
> 418   }
> 419   int32_t bodyWidth = 0, bodyHeight = 0;
> 420   if (bodyDOMElement) {
> 421     bodyDOMElement->GetScrollWidth(&bodyWidth);
> 422     bodyDOMElement->GetScrollHeight(&bodyHeight);
> 423   }
> 424 
> 425   float pageWidth = NS_MAX(htmlWidth, bodyWidth);
> 426   float pageHeight = NS_MAX(htmlHeight, bodyHeight);
https://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#411

So pageWidth and pageHeight are the height of our document's <html> and/or <body> nodes. SVG doesn't have either of those nodes, of course, so these variables are left at 0 for SVG.

Then we do:
> 453   metrics.mScrollableRect = gfx::Rect(0.0f, 0.0f, pageWidth, pageHeight);

... so for SVG, that gives us a 0,0,0,0 scrollable rect, which sounds pretty likely to make us paint nothing.  Not looking good.

Sure enough, we then call AsyncPanZoomController::CalculatePendingDisplayPort() and intersect that scrollableRect with something else to generate our (empty) displayPort as follows:
> 872   displayPort = shiftedDisplayPort.Intersect(aFrameMetrics.mScrollableRect);

...and that displayPort ends up getting used for the SetDisplayPortForElement() call in comment 13, which ultimately gives us the empty-dirtyrect BuildDisplayList call from comment 12.

Comment 15

[Daniel Holbert [:dholbert]]

[Marking as depending on bug 746502, which added the first chunk of code from prev. comment]

Comment 16

[Daniel Holbert [:dholbert]]

...and if I just blindly do the same dance with document->GetRootElement(), I still get (0,0), because nsGenericElement::ScrollWidth() and ::ScrollHeight() have early-returns for SVG, saying "if (IsSVG()) return 0;"

Maybe if we don't have a <body> or an <html> element, we should just use the full viewport. Even if that's overkill (and I don't think it is), we're just using it as a measure of the "scrollable region" of the page, and it's an accurate measure for 100%-by-100%-sized SVG content.  (Plus, we intersect it with another rect before determining our actual display port, so we're not even necessarily drawing this whole area if we don't have to -- just the intersection.)

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This fixes it. I added two XXX comments about possible divide-by-0's in neighboring code, too. (I'll probably file followups for those -- in cases where they'd actually occur, I'm not sure it'd actually cause user-visible issues, since there probably already wouldn't be anything drawn due to the page being 0-sized. However, still good to avoid dividing by 0...)

Comment 18

[Daniel Holbert [:dholbert]]

(Just to be clear: I've verified that "fix v1" resolves the issue w/ the attached testcase, on my unagi device)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 681905 [details] [diff] [review]
fix v1

># HG changeset patch
># Parent 303a143077633076a571b674d9600b33fed57c10
># User Daniel Holbert <[e-mail]>
>Bug 807457: In non-HTML documents, use the viewport size as the scrollable area, in TabChild::HandlePossibleViewportChange. r?smaug
>
>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
>@@ -417,22 +417,31 @@ TabChild::HandlePossibleViewportChange()
>     htmlDOMElement->GetScrollHeight(&htmlHeight);
>   }
>   int32_t bodyWidth = 0, bodyHeight = 0;
>   if (bodyDOMElement) {
>     bodyDOMElement->GetScrollWidth(&bodyWidth);
>     bodyDOMElement->GetScrollHeight(&bodyHeight);
>   }
> 
>-  float pageWidth = NS_MAX(htmlWidth, bodyWidth);
>-  float pageHeight = NS_MAX(htmlHeight, bodyHeight);
>+  float pageWidth, pageHeight;
>+  if (htmlDOMElement || bodyDOMElement) {
>+    pageWidth = NS_MAX(htmlWidth, bodyWidth);
>+    pageHeight = NS_MAX(htmlHeight, bodyHeight);
>+  } else {
>+    // For non-HTML content (e.g. SVG), just assume page size == viewport size.
>+    pageWidth = viewportW;
>+    pageHeight = viewportH;
>+  }
I guess this could work quite well usually, but depending on the ux we want, 
this may need tweaking later. The behavior is really up to b2g people to decide.


> 
>+  // XXXdholbert Uh oh, this could be divide-by-0...
Could you fix / 0 problem while you're here.
NS_ENSURE_TRUE_VOID(pageWidth);

>   minScale = mInnerSize.width / pageWidth;
>   minScale = clamped((double)minScale, viewportInfo.minZoom, viewportInfo.maxZoom);
> 
>+  // XXXdholbert Uh oh, this could be divide-by-0...
Could you fix / 0 problem while you're here.
NS_ENSURE_TRUE_VOID(minScale);


I think we're in odd situation if either one is 0 and just returning is ok.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: fix v2 [r=smaug]

OK, I added NS_ENSURE_TRUE_VOID() calls for each of those possibly-0 variables, right after we establish their value.

Carrying forward r=smaug and smaug's r?drs.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 682036 [details] [diff] [review]
fix v2 [r=smaug]

Not sure about Doug's availability at the moment, so I'm r?'ing cjones as well. 

(smaug says on IRC that he'd like one of them to sign off on it, too -- so, r?'ing both of them, in the hopes that one of them can get to it soonish.)

Comment 22

[Daniel Holbert [:dholbert]]

Thanks Doug!

Comment 23

[Daniel Holbert [:dholbert]]

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/514bf7203671

Comment 24

[Daniel Holbert [:dholbert]]

RyanVM tells me the "blocking-basecamp+" gives this implicit approval to land on Aurora.

I intend to backport this to Aurora in the next day or two, after it's proved itself on central.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/514bf7203671

Comment 26

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2cc32091e934

Comment 27

[Daniel Holbert [:dholbert]]

Verified fixed in latest B2G stable dogfood build:
Build Identifier 20121127132302