521189 - some necko unit tests fail or hang with 'strict' & 'werror' on

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jae-Seong Lee-Russo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre (.NET CLR 3.5.30729)

test_bug3890994.js fails:
TEST-UNEXPECTED-FAIL | c:\mozilla-build\mozilla-central\_tests\xpcshell\test_nec
ko\unit\test_bug380994.js | test failed (with xpcshell return code: 0), see foll
owing log:
  >>>>>>>
  TEST-INFO | (xpcshell/head.js) | test 1 pending
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | TypeError: assignment to undeclared
variable uri

  <<<<<<<

Tests that hang are:
test_necko/unit/test_authentication.js
test_necko/unit/test_bug263127.js
test_necko/unit/test_content_sniffer.js
test_necko/unit/test_cookie_header.js
test_necko/unit/test_event_sink.js
test_necko/unit/test_gzipped_206.js
test_necko/unit/test_redirect_caching.js


Reproducible: Always

Steps to Reproduce:
1. Run the unit tests with the '-S' option
Actual Results:  
Failed or Hang

Expected Results:  
Passed

Comment 1

[Jae-Seong Lee-Russo]

Attachment: Fix test_bug3890994.js

Comment 2

[Jae-Seong Lee-Russo]

Attachment: Convert to UNIX newline

Comment 3

[Jason Duell]

Jae,

The fix for test_bug3890994.js looks good.  We generally don't take a patch until it completely fixes a bug.  Do you want to try figuring out why the other tests are hanging?  There's a very high likelihood that they are hitting variables that haven't been declared, but that they're doing so after do_test_pending() has been called (so the error happens later, and thus do_test_finished() is never called and the test hangs. see https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests for details on how those functions work).

I've got a patch that lets you hit control-C when a test is hung, and you'll actually see whatever it printed up until the time it hung.  I recommend applying it (it's the latest patch for bug 520649) and seeing if you can tell what's going on.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 405443 [details] [diff] [review]
Convert to UNIX newline

>diff --git a/netwerk/test/unit/test_bug380994.js b/netwerk/test/unit/test_bug380994.js

> function run_test() {
>   var ios = Cc["@mozilla.org/network/io-service;1"].
>             getService(Ci.nsIIOService);
> 
>   for each (spec in specs) {
>-    uri = ios.newURI(spec,null,null);
>+    let uri = ios.newURI(spec,null,null);
>     if (uri.spec.indexOf("..") != -1)
>       do_throw("resource: traversal remains: '"+spec+"' ==> '"+uri.spec+"'");
>   }
> }

This is very high on the scale of nitpickiness, but it's well worth noting as something that will reoccur again and again as you fix bugs: follow the style of existing code when you make changes.  (The exceptions would be if your change is to change that code style, or perhaps if the existing code is so inconsistent that it can be said to have no such style.  Neither of those is the case here, if memory serves me right.)  This test uses const or var, never let, so this should be a var rather than a let.  I fixed that when I pushed just now:

http://hg.mozilla.org/mozilla-central/rev/efaab8e8f381

Thanks again!

Comment 5

[Jae-Seong Lee-Russo]

Attachment: Replace Ci.nsIThread.Manager.DISPATCH_NORMAL to gThreadManager.currentThread.DISPATCH_NORMAL

I turned on the DEBUG mode of httpd.js and was testing 'test_authentication.js'.

dumpn(Ci.nsIThreadManager.DISPATCH_NORMAL) returns undefined, so I applied this patch (gThreadManager.currentThread.DISPATCH_NORMAL is 0), ran the test_authentication.js again, and the test passes.  What do you think?

Comment 6

[Jae-Seong Lee-Russo]

Attachment: httpd.js, test_redirect_caching.js

test_resumable_channel.js fails with the '-S' option:

*** test_auth()
TEST-UNEXPECTED-FAIL | c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw3
2/_tests/xpcshell/test_necko/unit/test_resumable_channel.js | 2152398880 == 2152
398873 - See following stack:
JS frame :: c:\mozilla-build\mozilla-central-devel\testing\xpcshell\head.js :: d
o_throw :: line 197
JS frame :: c:\mozilla-build\mozilla-central-devel\testing\xpcshell\head.js :: d
o_check_eq :: line 227
JS frame :: c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xp
cshell/test_necko/unit/test_resumable_channel.js :: test_auth :: line 223
JS frame :: c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xp
cshell/test_necko/unit/head_channels.js :: anonymous :: line 106
TEST-INFO | (xpcshell/head.js) | exiting test

Comment 7

[Jae-Seong Lee-Russo]

Attachment: Fix

If the patch is fine, then all the necko tests pass.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

+  }, Ci.nsIThread.DISPATCH_NORMAL);

shouldn't that be nsIEventTarget?

Comment 9

[Jae-Seong Lee-Russo]

> shouldn't that be nsIEventTarget?

nsIThread inherits from nsIEventTarget, and the value returned correct (0), so I used nsIThread.  Is it wrong?  Please let me know.

> If the patch is fine, then all the necko tests pass.

Please ignore that.  I did not run tests under netwerk/test/httpserver/test and some tests need fix, too :(

Comment 10

[Jae-Seong Lee-Russo]

Attachment: Try to fix strict errors of the tests under /netwerk/test/, 1

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 409171 [details] [diff] [review]
Try to fix strict errors of the tests under /netwerk/test/, 1

>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js
>       gThreadManager.currentThread
>-                    .dispatch(stopEvent, Ci.nsIThreadManager.DISPATCH_NORMAL);
>+                    .dispatch(stopEvent, Ci.nsIThread.DISPATCH_NORMAL);

>           if (fis instanceof Ci.nsISeekableStream)
>-            fis.seek(Ci.nsISeekableStream.SEEK_SET, offset);
>+            fis.seek(Ci.nsISeekableStream.NS_SEEK_SET, offset);

>       function writeMore()
>       {
>         gThreadManager.currentThread
>-                      .dispatch(writeData, Ci.nsIThreadManager.DISPATCH_NORMAL);
>+                      .dispatch(writeData, Ci.nsIThread.DISPATCH_NORMAL);
>       }

>         }
>-      }, Ci.nsIThreadManager.DISPATCH_NORMAL);
>+      }, Ci.nsIThread.DISPATCH_NORMAL);
>     }

>       };
>     gThreadManager.currentThread
>-                  .dispatch(cancelEvent, Ci.nsIThreadManager.DISPATCH_NORMAL);
>+                  .dispatch(cancelEvent, Ci.nsIThread.DISPATCH_NORMAL);
>   },

Yeesh, that these (and the instances in other files) only happen to work is just ugly.  Pushing to m-c momentarily...

Comment 12

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/02c7eaa58fb8

Comment 13

[Nochum Sossonko [:Natch]]

Pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e0d4561865c1

Comment 14

[Nochum Sossonko [:Natch]]

Clearing checkin keywords since these patches don't have 1.9.1 approval. Please request approval on the patches, and once granted, replace the checkin keyword.