725679 - "suite/common/tests/browser/browser_346337.js | Test timed out" : "WARNING: RestoreFromHistory failed" + "ASSERTION: aIndex is out of range: 'aIndex < mLength'"

Status: VERIFIED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328740940.1328745964.22504.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/08 14:42:20
{
...
TEST-PASS | chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js | The value for "//input[@type='file'][2]" was correctly restored

WARNING: RestoreFromHistory failed: file e:/builds/slave/comm-cen-trunk-w32-dbg/build/mozilla/docshell/base/nsDocShell.cpp, line 6755
###!!! ASSERTION: aIndex is out of range: 'aIndex < mLength', file e:/builds/slave/comm-cen-trunk-w32-dbg/build/mozilla/docshell/shistory/src/nsSHistory.cpp, line 957

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js | Test timed out
}

PS: Reproduced locally while fixing bug 725529.

***

Then this test "leaks" into following ones, as in
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_522545.js | an unexpected uncaught JS exception reported through window.onerror - ok is not defined at chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js:141
}

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch [Checkin: Comments 13, 18 and 19]

This is what I figured from the main difference between our test and FF's.

Note: I haven't checked the follow-up errors, but only running this test alone.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

I confirm this fixes this bug locally :-)
But I have no idea why this test used some 'pageshow' (in SM) instead of 'load' (in FF) in the first place.

While looking at this patch, I noticed and filed bug 726505, which I would like to do first, just in case.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> While looking at this patch, I noticed and filed bug 726505, which I would
> like to do first, just in case.

This patch is still needed, even after fixing bug 726505.

Comment 4

[[e-mail]]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

This test and the Firefox one look substantially different, even with the event change as given in the patch, so I can't tell how our test works at all, sorry.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Serge, I guess either you need to turn your f+ into an r+, or we need to find another reviewer (who?), or remove this test.

Comment 6

[Philip Chee]

Perhaps Misak can remember why the tests differ so much?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #4)
> I can't tell how our test works at all, sorry.

Can you give a rs+, as you reviewed bug 480109?
(Or suggest another reviewer?)


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #5)
> Serge, I guess either you need to turn your f+ into an r+

Well, I don't think/remember I'm allowed to give r+ (on anything).
Ian, correct me if I'm wrong.


(In reply to Philip Chee from comment #6)
> Perhaps Misak can remember why the tests differ so much?

Yeah, but it looks like Misak hasn't been active/responsive since last november 2011 meeting :-/

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Serge Gautherie (:sgautherie) from comment #7)
> (In reply to Jens Hatlak (:InvisibleSmiley) from comment #5)
> > Serge, I guess either you need to turn your f+ into an r+
> 
> Well, I don't think/remember I'm allowed to give r+ (on anything).

If that is true (it might be), I think this needs to be changed, considering that you probably have the most test experience and the FF guys even commit stuff with "a=testonly" and no (obvious) review at all. If we have failing tests and someone (e.g. you) can tell that a fix is correct, by all means that should suffice (IMHO). Writing new tests is probably another story, though, but that's not what I'm up to here.

Comment 9

[Philip Chee]

> If that is true (it might be), I think this needs to be changed, considering that you
> probably have the most test experience and the FF guys even commit stuff with
> "a=testonly" and no (obvious) review at all.

And I've seen Firefox test only changes go in without even a bug number. So given Serge's experience with tests I think he is already a de facto seamonkey-tests peer so he can give r+ if needed.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #9)
> So given Serge's experience with tests I think he is already a de facto
> seamonkey-tests peer so he can give r+ if needed.

Ian, if you think this needs a vote, please take this up with the Council, otherwise integrate into what you are working on. Thanks!

Comment 11

[Ian Neal]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10)
> (In reply to Philip Chee from comment #9)
> > So given Serge's experience with tests I think he is already a de facto
> > seamonkey-tests peer so he can give r+ if needed.
> 
> Ian, if you think this needs a vote, please take this up with the Council,
> otherwise integrate into what you are working on. Thanks!

This doesn't need a vote, I am happy for Serge's f+ = r+

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

Then, even if we don't (bother to) have a full understanding of this test code,
this patch fixes the failure and makes the test somewhat closer to Firefox code,
so f+ -> r+.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

http://hg.mozilla.org/comm-central/rev/650205a133cd

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

[Approval Request Comment]
Test-only, wanted to clear the timeout and the rest.

Comment 15

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329271174.1329275384.15583.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2012/02/14 17:59:34

V.Fixed, test + warning + assertion + etc!

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

WoooHooo clearing timeouts means more time to spend on the machines actually doing stuff!

p.s.: I agree with
|So given Serge's experience with tests I think he is already a de facto seamonkey-tests peer so he can give r+ if needed.|

Comment 17

[Philip Chee]

Err he's now a de jure seamonkey-tests peer.
http://www.seamonkey-project.org/dev/project-areas
https://wiki.mozilla.org/Modules/SeaMonkey#Cross-Suite_Components

Comment 18

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/comm-beta/rev/67d5e945c579

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 596340 [details] [diff] [review]
patch [Checkin: Comments 13, 18 and 19]

http://hg.mozilla.org/releases/comm-aurora/rev/4138254af9bf

Comment 20

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1331173545.1331178886.17486.gz
Linux comm-aurora debug test mochitest-other on 2012/03/07 18:25:45

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1331186022.1331189779.1265.gz
OS X 10.5 comm-beta debug test mochitest-other on 2012/03/07 21:53:42

seamonkey2.9 and seamonkey2.8: verified.