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

VERIFIED FIXED in Firefox 18

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({b2g-testdriver, unagi})

Trunk
mozilla19
ARM
Gonk (Firefox OS)
b2g-testdriver, unagi
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 677152 [details]
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
(Assignee)

Updated

6 years ago
Summary: [Browser] SVG content not repainted when switching back to a tab from tab-preview view → [Browser] SVG content not repainted, when coming back from tab listing
(Assignee)

Comment 2

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

Updated

6 years ago
Summary: [Browser] SVG content not repainted, when coming back from tab listing → [Browser] SVG content not repainted, when coming back from tab listing, or navigating back to it w/ "back" button
related to bug 806035?
(Assignee)

Comment 4

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

Comment 5

6 years ago
...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)
Component: Gaia::Browser → General
roc, any ideas about what could be causing this?
blocking-basecamp: ? → +
Flags: needinfo?(roc)
No. Are we hitting _cairo_error?
Flags: needinfo?(roc)

Comment 8

6 years ago
Daniel, can you answer #7?
Flags: needinfo?(dholbert)
(Assignee)

Comment 9

6 years ago
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.
Flags: needinfo?(dholbert)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Assignee)

Comment 10

6 years ago
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
Try logging with DEBUG_INVALIDATIONS?
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

6 years ago
[Marking as depending on bug 746502, which added the first chunk of code from prev. comment]
Depends on: 746502
(Assignee)

Comment 16

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

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 17

6 years ago
Created attachment 681905 [details] [diff] [review]
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...)
Attachment #681905 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Component: General → DOM: Core & HTML
Product: Boot2Gecko → Core
Version: unspecified → Trunk
(Assignee)

Comment 18

6 years ago
(Just to be clear: I've verified that "fix v1" resolves the issue w/ the attached testcase, on my unagi device)
Comment on attachment 681905 [details] [diff] [review]
fix v1

># HG changeset patch
># Parent 303a143077633076a571b674d9600b33fed57c10
># User Daniel Holbert <dholbert@cs.stanford.edu>
>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.
Attachment #681905 - Flags: review?(bugzilla)
Attachment #681905 - Flags: review?(bugs)
Attachment #681905 - Flags: review+
(Assignee)

Comment 20

6 years ago
Created attachment 682036 [details] [diff] [review]
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.
Attachment #681905 - Attachment is obsolete: true
Attachment #681905 - Flags: review?(bugzilla)
Attachment #682036 - Flags: review?(bugzilla)
(Assignee)

Comment 21

6 years ago
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.)
Attachment #682036 - Flags: review?(jones.chris.g)
Attachment #682036 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

6 years ago
Attachment #682036 - Flags: review?(jones.chris.g)
(Assignee)

Comment 22

6 years ago
Thanks Doug!
(Assignee)

Comment 24

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/514bf7203671
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 26

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/2cc32091e934
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(Assignee)

Comment 27

6 years ago
Verified fixed in latest B2G stable dogfood build:
Build Identifier 20121127132302
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.