589396 - Tgfx is broken and 130078 exposes it

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Timothy Nikkel (:tnikkel)]

Tgfx_nochrome absolutely blows up with bug 130078, see the links in bug 588663. There are 10000%+ increases.

The pageloader extension in pageloader.js picks a window with this code in runRenderTest
  if (browserWindow)
    win = content.contentWindow;
  else 
    win = window;
and then calls nsIDOMWindowUtils::Redraw 500 times on win. For Tgfx_chrome the if condition is always true, for Tgfx_nochrome it is always false due to how the test is setup. In the Tgfx_chrome case win contains the pages we are measuring the performance of (ie borders-dashed.html). In the Tgfx_nochrome case win is the parent chrome window (ie pageloader.xul) which contains a child document that loads the pages we are testing.

Now, all nsIDOMWindowUtils::Redraw does is get the root frame of the window and call InvalidateWithFlags(r, nsIFrame::INVALIDATE_IMMEDIATE) on it where r is the size of the root frame. nsIFrame::InvalidateWithFlags checks if painting is suppressed in the presshell, and if it is returns and doesn't do anything.

The 500 nsIDOMWindowUtils::Redraw calls happen off of the load event for the child content document. DocumentViewerImpl::LoadComplete is where the load event is dispatched, and it unsuppresses painting after it dispatches the load event.

So in Tgfx_chrome case the 500 nsIDOMWindowUtils::Redraw calls happen off of the load event of the child content document, and are targeted at the child content document. So when they happen painting is suppressed and they all do nothing.

In the Tgfx_nochrome case the 500 nsIDOMWindowUtils::Redraw calls happen off of the load event of the child content document, but are targeted at the root chrome document. The root chrome document has not just loaded so its painting is not suppressed. But there is nothing to paint in the root chrome document, so it is still quick. This explains why Tgfx_nochrome takes longer than Tgfx_chrome.

When we add the patches from bug 130078 the view managers for the chrome and content documents are linked, so the paint events targeted at the root chrome document also paint the child content document (we don't currently suppress painting when entering a subdocument that has painting suppressed). And this takes a lot longer since it is actually doing some work.

Comment 1

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

Clearly we need to fix Tgfx, but I don't think this should block 130078 landing.

Comment 2

[Timothy Nikkel (:tnikkel)]

For a quick "fix" we can just restore the broken behaviour so the numbers stay about the same by making the window we send the Redraw's to be the content window in the nochrome case as well.

Comment 3

[alice nodelman [:alice] [:anode]]

Is this a version of bug 493050 ?

Comment 4

[Timothy Nikkel (:tnikkel)]

Closely related. If we don't change anything then bug 130078 is going to massively regress the Tgfx_nochrome numbers. Bug 130078 needs to land soon. So we need to do something about that. We can either disable the test. Or we can make a small change to the testing harness so that it continues to be bogus, but gives approximately the same numbers. We can decide what we want to do with Tgfx longer term in bug 493050, but we need a quick solution here.

Comment 5

[Joe Drew (not getting mail)]

I think spending any time at all making Tgfx results look sane is time wasted. Send an email to dev.tree-management saying "please ignore this apparent Tgfx regression" and move on. We can work out what to do with Tgfx in bug 493050.

Comment 6

[alice nodelman [:alice] [:anode]]

I would also suggest that we should disable tgfx till it actually does something useful - right now we are just wasting cpu time on running it for every branch on every OS for every push.

Comment 7

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

Yes please, can you do that?

Comment 8

[Chris AtLee [:catlee]]

Attachment: Disable tgfx

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 471348 [details] [diff] [review]
Disable tgfx

Checked in as changeset:   2932:c0b16bcdc30b, and masters reconfigured.

Comment 10

[Timothy Nikkel (:tnikkel)]

Should we track fixing Tgfx in bug 493050 and mark this resolved?

Comment 11

[Aki Sasaki (not active)]

https://bugzilla.mozilla.org/show_bug.cgi?id=493050#c15