379233 - Pageload tests should flush out layout before timing load end

Status: RESOLVED FIXED

(Testing :: General, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

See bug 379093.  I don't really think we want the pageload tests showing a time drop when bugs like that are introduced.

Unfortunately, I don't have access to the Tp code, or even the code that's used for Tp2 on the tinderboxen.  Attached patch is against the "public" Tp2 version that's in the tree.

Comment 1

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

I think that this can be integrated with the new perf tests - but I'm unsure what the standard tinderboxes are currently running.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Alice, I just cced you for the new perf tests.  rhelmer's my target for the existing ones.  ;)

Comment 3

[Adam Guthrie]

FWIW, the only difference between the Tp2 that the tinderboxes are running and the one in the tree is the list of pages in the |pages| variable (which would be moved to an external file if we land the patch in bug 359994).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I know.  But the point is that if I change the in-tree version the tinderboxes won't pick it up automatically...

Comment 5

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

Can that check be done after the timer is stopped?  I don't want this to effect the pageload time for correct pages.  Otherwise I think that I can get the suggested fix checked in with some other cycler improvements that I've been working on.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Which "that check"?  And which timer?

What do you mean by "correct pages"?

And what is your definition of "pageload time"?

Comment 7

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

I'm referring to:

var offsetWidth = frames["content"].document.documentElement.offsetWidth;

From the code chunk you provided it compares start and end time after this call and calculates the page load time from that.  I'm wondering if we can do the offsetWidth test after collecting the loadtime - just in case the call to offsetWidth causes a slow down in the page load time collected.

My concern being that if this gets integrated into the cycler code that it could cause an increase in the numbers.  I guess that that really isn't such a big deal, as long as it is a consistent increase across the board, but I'm wary of affecting the page load times for successfully loaded pages.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Again, what do you mean by "page load time"?

On trunk, we guarantee that layout is done by the time onload fires.  In the past, we had bugs so that we didn't guarantee it.  We want to do apple-to-apple comparisons of perf on trunk and branch.  Which means that the test itself needs to guarantee that layout is done before the timer stops.  The line of code in question does that.

So the whole point of the change is to get the offsetWidth before we stop timing.  On trunk I expect it to have 0 effect on the numbers, unless something is _very_ wrong.  On branches, depending on the bugginess of them, it might have an effect, but that effect is exactly what we want.

Comment 9

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

Moving to correct talos category.

Comment 10

[[e-mail] (:bhearsum)]

This has been sitting for a long time - moving to Future.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Honestly, I'm not sure why "it's taken us a long time to fix this" is good justification for "we should take even longer".  :(  It's an attitude I hate for our browser product too...

Comment 12

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

Is that patch still current?  

If so, this would need baking time on stage to see what happens to perf numbers and if it doesn't fail (the patch there seems to suggest that there is a fail state that could happen semi-frequently, but I could be wrong).

Comment 13

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

Attachment: check offset before recording page load complete

The old patch deals with the old tp infrastructure, we only do tp tests using the pageloader extension now.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Alice, thanks for moving the fix over to the new infrastructure.  That looks great!  If we ever need to do non-HTML here, it's not hard to modify things to use something like getBoundingClientRect, which will work for all languages.

Comment 15

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

With that patch it will just ignore any error and keep testing - do you want that to be a real fail state that stops the test pages from cycling?

Comment 16

[Boris Zbarsky [:bzbarsky]]

If possible, that would be great.

Comment 17

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

Attachment: check offset before recording page load complete, stop if check fails

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, the patch looks fine, but I think that we already have some non-html tests in the pageloader, e.g. Tsvg.  Those tests will break, no?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm.. actually, offsetWidth would just return undefined, and no breakage.  But yeah, doing getBoundingClientRect might be better.

Comment 20

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

What would the less-breaky check look like?  I'd be happy to update the patch with it.

Comment 21

[Boris Zbarsky [:bzbarsky]]

This should work for all our branches (HTML-only on 1.8, works for everything on 1.9 and newer):

  var docElem = frames["content"].document.documentElement;
  var width;
  if ("getBoundingClientRect" in docElem) {
    width = docElem.getBoundingClientRect().width;
  } else if ("offsetWidth" in docElem) {
    width = docElem.offsetWidth;
  }

No need for the try/catch + fail if we're using this approach, I think.

Comment 22

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

Attachment: [Backed out]check offset before recording page load complete (attempt #3)

Updated patch with bz's suggested fix.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 353901 [details] [diff] [review]
[Backed out]check offset before recording page load complete (attempt #3)

Yep, looks good!

Comment 24

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

Comment on attachment 353901 [details] [diff] [review]
[Backed out]check offset before recording page load complete (attempt #3)

Checking in pageloader.js;
/cvsroot/mozilla/layout/tools/pageloader/pageloader.js,v  <--  pageloader.js
new revision: 1.15; previous revision: 1.14
done

Comment 25

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

Comment on attachment 353901 [details] [diff] [review]
[Backed out]check offset before recording page load complete (attempt #3)

This patch is causing frozen browsers on talos boxes, backed out.

Comment 26

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #25)
> (From update of attachment 353901 [details] [diff] [review])
> This patch is causing frozen browsers on talos boxes, backed out.

urr... what next to do here?

Comment 27

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 353901 [details] [diff] [review] [details])
> > This patch is causing frozen browsers on talos boxes, backed out.
> 
> urr... what next to do here?

bz; want to try this again or should we just close?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Do we have any sort of output here?  As in, was there an exception thrown?  Or was this an actual freeze (in the sense of process not responding to input)?

I do still think we need to fix this bug.

I'd also love any sort of response to the talos issue I recently raised in .platform, which is related to this bug.

Comment 29

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

It froze as in did not advance to the next page in the manifest - eventually Talos notices the lack of inactivity and kills off the browser and reports as 'frozen'.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Can we try wrapping a try/catch around this code with a dump() of the exception in the catch clause?  It sounds like an exception was thrown by the added code for some reason...

Comment 31

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

I think that what we need is somebody to be assigned this bug so that the code can be tested locally to ensure that it is working correctly.  I'd like to pick it up, but I don't think that I have time in my schedule for it right now.

What sort of priority do we want to put on this?

Comment 32

[Boris Zbarsky [:bzbarsky]]

P3 (as now) is probably about right, if my opinion matters.

Comment 33

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

Attachment: check offset before recording page load complete (attempt #4)

Pretty much the same as before, but we were checking the offset of the wrong window.  Specified now to check browserWindow - the window that is actually cycling through pages.  Tested on my home machine and it actually advances through the pages instead of getting frozen.

Would still require a downtime for landing to ensure that a) there's nothing that I'm missing and b) in case it alters recorded tp numbers.

Comment 34

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

Comment on attachment 362168 [details] [diff] [review]
check offset before recording page load complete (attempt #4)

Checking in pageloader.js;
/cvsroot/mozilla/layout/tools/pageloader/pageloader.js,v  <--  pageloader.js
new revision: 1.17; previous revision: 1.16
done

Comment 35

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

Comment on attachment 362168 [details] [diff] [review]
check offset before recording page load complete (attempt #4)

Backed out, froze up Firefox3.0 talos boxes.

Comment 36

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

Moving to future till either I have time to work on this or a patch presents itself.

Comment 37

[Chris Cooper [:coop] (he/him)]

Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.

Comment 38

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

found in triage. 

alice is this still needed?

Comment 39

[Boris Zbarsky [:bzbarsky]]

This is still needed to do useful cross-branch comparisons, yes.

Also if we ever decide to change our core behavior to not flush right before onload, of course.

Comment 40

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

Since this only showed problems on 1.9.0 and 1.9.0 is end-of-life-ing we could attempt a reland after that branch is discontinued... assuming that it doesn't show problems on any of our new branches.

Comment 41

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Found during triage. Adding depbug, so we can investigate after 1.9.0 stops.

Comment 42

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

This is about changes in a talos suite, not the automation of a suite, so moving this bug to Testing:General as requested by bmoss, ctalbert.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Also if we ever decide to change our core behavior to not flush right before
> onload, of course.

Which we want to do.  See bug 572604.  So at this point this is blocking core product changes.

Comment 44

[Boris Zbarsky [:bzbarsky]]

I wasn't willing to hold said core changes for this bug given its history; they needed to happen for the beta.  So starting tonight, Tp4 on m-c is ... probably less useful than it should be.  :(

Comment 45

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

We no longer test 1.9.0 with talos (bug 574062), so this should be landable now.

Comment 46

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

To get this to roll out:

- need a patch against bsmedberg's pageloader as bundle (which is what talos currently uses to install the pageloader)
- quick staging run
- downtime

Comment 47

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

Testing patch on talos-staging-master02 on mozilla-central, mozilla-1.9.1 and mozilla-1.9.2.

Comment 48

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

Attachment: re-based patch based upon bsmedberg's pageloader bundle

Comment 49

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

I'm seeing green runs on mozilla-central, 1.9.1 and 1.9.2.  No freezing
observed, which was what caused the back out of this patch last time around. 
There were some red runs, but they were due to crashes or other non-related
failures.

Comment 50

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

I took a closer look at the logs and the patch breaks tgfx - I'll look into this and see if it can be fixed.

Comment 51

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

Attachment: re-based patch based upon bsmedberg's pageloader bundle + fix freezing state

Previous patch didn't take into account nochrome tests which do not create a secondary browserWindow.  New code handle both chrome/nochrome case.

Still don't have an error state here, but just running the check and not freezing would be a win considering the age of this bug.

Comment 52

[Chris Cooper [:coop] (he/him)]

Comment on attachment 471322 [details] [diff] [review]
re-based patch based upon bsmedberg's pageloader bundle + fix freezing state

http://hg.mozilla.org/build/pageloader/rev/b4da709724e1

Comment 53

[Chris Cooper [:coop] (he/him)]

Also updated http://build.mozilla.org/talos/xpis/pageloader.xpi