Closed Bug 511096 Opened 15 years ago Closed 13 years ago

SeaMonkey/SMILE port of FUEL Bug 458688 (occasional browser_Browser.js timeouts)

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.0

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: fixed-seamonkey2.0, Whiteboard: [cc-orange])

Attachments

(3 files, 1 obsolete file)

Patch v1.1 [Checkin: Comment 5]
6.09 KB, patch
neil
: review+
Details | Diff | Splinter Review
Patch v2.0 Sync with Firefox test[Checkin: Comment 30].
4.37 KB, patch
neil
: review+
sgautherie
: feedback+
Details | Diff | Splinter Review
(Cv1-FF) Trivial sync' to FF from SM
1.56 KB, patch
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #458688 +++

[quote]
The test in the URL field uses setTimeout extensively. That's bogus, and can cause random failures as shown here:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1223284948.1223288480.11938.gz
[/quote]
Attached patch Patch v1.0 Port changes to test. (obsolete) — — Splinter Review 
I ran a diff between the FUEL trunk test and our current version to generate an initial draft of this patch. This is a straight port except for:

+  // nsIFocusManager is not available on MOZILLA_1_9_1
+  if ("nsIFocusManager" in Ci &&
+      Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)
+                                        .activeWindow != window) {
+    setTimeout(test, 0);
+    window.focus();
+    return;
+  }

I don't know how to do this in 1.9.1 as the FUEL patch hasn't been backported. But with this IFfed out I get all the tests passing:

Browser Chrome Test Summary
        Pass: 93
        Fail: 0
        Todo: 0
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #395050 - Flags: review?(neil)
> +      Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)
> +                                        .activeWindow != window) {
activeWindow is a new concept. I'm not sure what the previous code was, although it may be responsible for the "Unable to restore focus, expect failures and timeouts." message.
And that was a simple document.hasFocus() call.
Depends on: 458688
> activeWindow is a new concept. I'm not sure what the previous code was,
....
> And that was a simple document.hasFocus() call.

document.hasFocus() still exists on mozilla-central.

I check for ("nsIFocusManager" in Ci) first then fall back to document.hasFocus() in separate blocks. This is more verbose but once we branch deleting one or the other is simpler.
Attachment #395050 - Attachment is obsolete: true
Attachment #397226 - Flags: review?(neil)
Attachment #395050 - Flags: review?(neil)
Attachment #397226 - Flags: review?(neil) → review+
Keywords: checkin-needed
Whiteboard: [NPOTDB test only]
Keywords: checkin-needed
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/9d71ad5cc15c
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
Attachment #397226 - Attachment description: Patch v1.1 → Patch v1.1 [Checkin: Comment 5]
Flags: in-testsuite+
Keywords: fixed-seamonkey2.0
Whiteboard: [NPOTDB test only]
Not (fully) fixed yet:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1252535209.1252542039.10685.gz
WINNT 5.2 comm-1.9.1 unit test on 2009/09/09 15:26:49
{
...
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Timed out
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1254040743.1254046531.22430.gz
Linux comm-1.9.1 unit test on 2009/09/27 01:39:03
Blocks: 438871
Whiteboard: orange
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255725237.1255727590.21947.gz#err0

Linux mozilla-central test opt everythingelse [testfailed] Started 13:33, finished 14:14
Blocks: 537898
Depends on: 473156
There are currently a few differences between ff and sm files:
I assume it couldn't hurt to port them (too).
> There are currently a few differences between ff and sm files:
> I assume it couldn't hurt to port them (too).

Are they mozilla-191 branch or mozilla-central differences?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266619173.1266620237.7128.gz
Linux mozilla-central opt test mochitest-other [testfailed] Started 14:39, finished 14:58
Bug still there:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1283307989.1283308336.2113.gz
OS X 10.5 comm-1.9.1 test mochitest-other on 2010/08/31 19:26:29
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1283704126.1283704529.1147.gz
OS X 10.5 comm-1.9.1 test mochitest-other on 2010/09/05 09:28:46
{
...
TEST-PASS | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking event handler for tab move
Document chrome://mochikit/content/browser/suite/smile/test/ContentWithFrames.html loaded successfully
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Timed out
}
Ouch, now we're getting Firefox tbpl posts here, that's not good :(
Summary: Port FUEL Bug 458688 (test browser_Browser.js still times out occasionally) → SeaMonkey/SMILE port of FUEL Bug 458688 (occasional browser_Browser.js timeouts)
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1290777777.1290778183.28205.gz
Linux comm-1.9.1 test mochitest-other on 2010/11/26 05:22:57
{
TEST-PASS | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking event handler for tab move
Document chrome://mochikit/content/browser/suite/smile/test/ContentWithFrames.html loaded successfully
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Timed out
}
No longer blocks: 438871
I'm getting tired of seeing this in my assigned list so I'm going to hijack this slightly and bring our test up to date with Firefox. This test passes locally.

Serge, do you think you could run this on the try server for me? Thanks.
Attachment #514783 - Flags: feedback?(sgautherie.bz)
Comment on attachment 514783 [details] [diff] [review]
Patch v2.0 Sync with Firefox test[Checkin: Comment 30].

Forgot to set review flag.
Attachment #514783 - Flags: review?(neil)
Comment on attachment 514783 [details] [diff] [review]
Patch v2.0 Sync with Firefox test[Checkin: Comment 30].

Only 3 "intentional" nit-differences remain:
I/we could try a trivial Firefox patch to sync' it with SM ;-|
Or we might just want to "undo" them to make the file 100% identical to Firefox one :-|

(In reply to comment #23)
> Serge, do you think you could run this on the try server for me? Thanks.

Submitted:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=3032777e72c1
Attachment #514783 - Flags: feedback?(sgautherie.bz) → feedback+
(In reply to comment #25)
> I/we could try a trivial Firefox patch to sync' it with SM ;-|

Let them be 100% identical.
Attachment #514993 - Flags: review?(dtownsend)
(In reply to comment #25)
> (In reply to comment #23)
> > Serge, do you think you could run this on the try server for me? Thanks.
> 
> Submitted:
> http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=3032777e72c1

Oh, I knew I hadn't used c-c Try for some time (and that I'm quite tired) :-(
1) I pushed the changeset (alone) ... which triggered a TB build, by default :-<
2) c-c Try doesn't run any mochitest ... so I'm not sure what you were looking for exactly :-(

Ftr, https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer
Attachment #514783 - Flags: review?(neil) → review+
> 2) c-c Try doesn't run any mochitest ... so I'm not sure what you were looking
> for exactly :-(
Oh arrgh. I didn't know that. Sorry!
I think it's a really good idea to backport improvements/nits to Firefox, and we should always do that, but you really should file a separate Firefox bug for those, this makes it much easier to handle for their and our tracking.
Comment on attachment 514783 [details] [diff] [review]
Patch v2.0 Sync with Firefox test[Checkin: Comment 30].

Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/678a973a7e1c
Attachment #514783 - Attachment description: Patch v2.0 Sync with Firefox test. → Patch v2.0 Sync with Firefox test[Checkin: Comment 30].
Closing. Serge, please open a new bug for test failures if/when they happen.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 13 years ago
Resolution: --- → FIXED
Comment on attachment 514993 [details] [diff] [review]
(Cv1-FF) Trivial sync' to FF from SM

What is the point of this patch?
Comment on attachment 514993 [details] [diff] [review]
(Cv1-FF) Trivial sync' to FF from SM

Removing review request to take it off SeaMonkey bug radars. Serge, you've been around long enough to know that you should file a separate bug in Firefox if you want to fix firefox.
Attachment #514993 - Flags: review?(dtownsend)
Whiteboard: orange → [cc-orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: