658758 - xpcshell broken for e10s due to weird escaping of \n in child output

Status: RESOLVED DUPLICATE of bug 645532

(Testing :: XPCShell Harness, defect)

Description

[Jason Duell]

Attachment: Workaround \n escaping by replacing with carriage returns.

Once again, xpcshell has been ignoring TEST-UNEXPECTED-FAIL for e10s xpcshell tests for some unknown amount of time.  This time it's because output from the child has carriage returns mysteriously replaced with the literal 2-char string "\n".

To reproduce:
    
    1) Run any of the tests in netwerk/test/unit_ipc, ex:

        make check-one SOLO_FILE=test_simple_wrap.js
   
    2) Observe that all the output marked "child" in on one line, with lots of
       '\n' where carriage returns ought to be

    3) Try putting in a failed expression "do_check_eq('mozilla' == 'webkit').
       Notice that runxpcshell doesn't notice the failure :(

I am totally unclear about why the child's carriage returns are getting replaced with '\n'.   I've tried a number of things:

    - Rolling back head.js and runxpcshell to quite a few months ago, when this
      was definitely not happening.  I still see the \n.

    - Using a file instead of PIPE for the python Popen call.  I still see the
      \n.

I'd suspect some change in our IPC infrastructure, but AFAICT dump() on the child doesn't travel over our IPC mechanisms (i.e. sendCommand())--it just goes to the process' stdout.  So I can't think of any reason why \n would be getting escaped all of a sudden.  Thoughts?

I suppose it's possible it's some python version issue--I'm running Python 2.6.6 on Ubuntu 10.10.  I'd be interested to see if others see the same behavior elsewhere.

I've attached a patch that works around the issue, by replacing '\nchild' with a carriage return and 'child'.  (This is conservative, and misses some escaped \n's, but seems preferable to a global \n replacement, which might catch things like Windows pathnames, etc.)

Also, since this is the second time now that e10s xpcshell tests have been missing test failures due to the strictness of the regular expression we use to detect failure, I've taken the liberty of change the regex so that it no longer requires TEST-UNEXPECTED- to be at the start of a line.   I think that's fine--tests shouldn't emit TEST-UNEXPECTED- without intending to fail.

Personally I think we should also get rid of the check for stdout before running the regex--if stdout is null, that should be a failure too, no?

I've run this patch through try, and no 'new' xpcshell failures seem to show up, i.e. no failures appear to have been actually hidden by this bug.   Lucky...

Comment 1

[Jason Duell]

cc-ing some IPC usual suspects, in case they've got any idea why carriage returns in the child's dump() calls might recently be getting escaped to literal '\n'.

Comment 2

[(not currently active) Ted Mielczarek]

(In reply to comment #0)
> Also, since this is the second time now that e10s xpcshell tests have been
> missing test failures due to the strictness of the regular expression we use
> to detect failure, I've taken the liberty of change the regex so that it no
> longer requires TEST-UNEXPECTED- to be at the start of a line.   I think
> that's fine--tests shouldn't emit TEST-UNEXPECTED- without intending to fail.

I guess this is okay, but a little weird.


> Personally I think we should also get rid of the check for stdout before
> running the regex--if stdout is null, that should be a failure too, no?

No, stdout can be null if you're running in interactive mode, or with an interactive debugger.

I don't really have anything to add about the underlying issue here, I don't really know that much about how the child xpcshell stuff works.

Comment 3

[Josh Matthews [:jdm]]

I'm 99% sure this is a dup of bug 645532, the patch for which was denied back in march since it didn't seem "super urgent".

Comment 4

[Jason Duell]

OK, asked for re-review of bug 645532, which is the better way to fix this.  Assuming that lands, we can mark this as a DUPE or WORKFORME or INVALID or whatever.

Comment 5

[Jason Duell]

Fixed the right way in bug 645532