Closed Bug 1163891 Opened 9 years ago Closed 9 years ago

xpcshell, in interactive mode, shows a new prompt every second after 5 seconds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: glandium, Assigned: Waldo)

References

Details

Attachments

(1 file, 1 obsolete file)

Found this while checking bug 1162187.

STR:
- Start xpcshell
- Wait 5 seconds on the js> prompt.

Result:
- After the initial 5 seconds, a new js> prompt is printed every second, so you end up with js> js> js> js> js> ...
That's quite odd.  That prompt is printed by the GetLine() function, which looks like this:

  static bool
  GetLine(JSContext* cx, char* bufp, FILE* file, const char* prompt) {
    {
        char line[4096] = { '\0' };
        fputs(prompt, gOutFile);
        fflush(gOutFile);
        if ((!fgets(line, sizeof line, file) && errno != EINTR) || feof(file))
            return false;
        strcpy(bufp, line);
    }
    return true;
  }

I do see the behavior, and if I attach gdb and then breakpoint on GetLine() and continue I see it get called.  It's being called from ProcessFile.  ProcessFile is NOT getting called after the breakpoint is set.  Furthermore, the print-every-second behavior disappears while gdb is attached (as expected, since the GetLine function doesn't actually return until the fgets returns, in theory).  As soon as I detach gdb the repeated printing starts happening again.

My best guess is that fgets is in fact returning with errno == EINTR for some reason.
Yep, adding some printfs confirms: the fgets is returning null, with errno == EINTR.

dtruss on Mac says that between repeated printing of the prompt we have this:

psynch_cvwait(0x114B1B4C8, 0x100100001100, 0x0)              = -1 Err#316
sysctl(0x116D03E10, 0x4, 0x116D03B88)            = 0 0
__pthread_kill(0x603, 0x1A, 0x116D03B88)                 = 0 0
gettimeofday(0x116D03D10, 0x0, 0x116D03B88)              = 1431402377 0
read_nocancel(0x0, "\0", 0x1000)                 = -1 Err#4
sigreturn(0x7FFF5FBFC990, 0x1E, 0x1000)          = 0 Err#-2
write_nocancel(0x1, "js> \0", 0x4)               = 4 0

That __pthread_kill is looking suspicious.  0x1A is presumably the signal number, which is VTALRM over here according to "kill -l".

So looks like this is fallout from bug 1091912, possibly?  We could probably fix xpcshell to handle this somehow, but it's a bit annoying to do that.  Unless we think any signal we use here will trigger the fgets to return?

I'm not sure why attaching gdb makes the problem go away, offhand.
Component: XPConnect → JavaScript Engine
Flags: needinfo?(luke)
This looks like spurious wakeup which isn't being handled correctly in the xpcshell code.  I think someone intended to handle this (the EINTR check) but forgot the loop.
Flags: needinfo?(luke)
> I'm not sure why attaching gdb makes the problem go away, offhand.

It doesn't on linux, fwiw.
(In reply to Not doing reviews right now from comment #2)
> Unless we think any signal we use here will trigger the fgets to return?

In general, anything doing I/O should be checking for EINTR cases and retry.
That particular EINTR appears to be caused by the JS Watchdog thread.
I think this is what we want, but I'm not super-versed in errno/EINTR semantics such that this exactly rolled off my fingers.  It does appear to do what I want in manual testing.
Attachment #8629118 - Flags: review?(mh+mozilla)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8629118 [details] [diff] [review]
Read lines of input in xpcshell while properly handling EINTR failures and retrying in response

Review of attachment 8629118 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +202,5 @@
> +            if (fgets(line, sizeof line, file))
> +                break;
> +            if (feof(file))
> +                return false;
> +        } while (errno == EINTR);

However unlikely, you're going to strcpy if fgets failed and errno is not EINTR, at which point the line buffer is stale.

Note that js/src/shell/js.cpp has different code for the same thing, and, afaict, has the same "not handling EINTR" problem.
Attachment #8629118 - Flags: review?(mh+mozilla) → review-
Attachment #8629196 - Flags: review?(mh+mozilla)
Attachment #8629118 - Attachment is obsolete: true
Comment on attachment 8629196 [details] [diff] [review]
Adjusted, with jsshell similarly fixed as well

Review of attachment 8629196 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +288,5 @@
>          fflush(gOutFile);
>      }
> +
> +    size_t size = 80;
> +    char* buffer = static_cast<char*>(malloc(size));

since this is js_realloc'ed, this should be js_malloc'ed. I know this has been like this for a while, but since you're touching here and it jumped to my eye, you might as well fix that ;). There's a free instead of js_free too.

@@ +298,5 @@
> +        while (true) {
> +            if (fgets(current, size - len, file))
> +                break;
> +            if (errno != EINTR)
> +                return nullptr;

You're leaking buffer here. Using a UniquePtr would probably be better, but just freeing would work too.

Mozilla Coding Style wants braces around those one-line conditional blocks, btw.
Attachment #8629196 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #10)
> > +    size_t size = 80;
> > +    char* buffer = static_cast<char*>(malloc(size));
> 
> since this is js_realloc'ed, this should be js_malloc'ed. I know this has
> been like this for a while, but since you're touching here and it jumped to
> my eye, you might as well fix that ;). There's a free instead of js_free too.

Switched everything to malloc/realloc/free rather than to js_malloc*.  The editline code returns info from the malloc heap, so the only way to be consistent is to use that everywhere.

Incidentally, I don't see anything at all that frees this memory, so I have no idea why valgrind doesn't complain about it leaking, or at least about allocator mismatches or so.  :-\

> @@ +298,5 @@
> > +        while (true) {
> > +            if (fgets(current, size - len, file))
> > +                break;
> > +            if (errno != EINTR)
> > +                return nullptr;
> 
> You're leaking buffer here. Using a UniquePtr would probably be better, but
> just freeing would work too.

Did a free.  If we did UniquePtr we'd need some sort of FreePolicy that hooked up to free(), and I don't quite think there's one sitting around for common use right now.  Easy to add, but I don't want to sidetrack the bug on that.
https://hg.mozilla.org/mozilla-central/rev/7389fc89e7aa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: