Closed Bug 1177488 Opened 9 years ago Closed 8 years ago

make AutoSetAsyncStackForNewCalls take a "const char*"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: tromey, Assigned: froydnj)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

In bug 1148593 we wanted to change AutoSetAsyncStackForNewCalls
(and SavedStack, etc) to take a "const char*" for the async cause,
rather than a JSString*.

See
https://bugzilla.mozilla.org/show_bug.cgi?id=1148593#c19
and some subsequent comments there.
Going to try and do this.
Assignee: nobody → nfroyd
This change was surprisingly easy to make, at the cost of touching many files.

There are two parts I am not completely confident in:

- The SavedStacks.cpp change now allocates JSStrings at the point of stack
  capture (good!), but introduces new failure modes.  I'm not sure the error
  handling is completely correct, or that this code is equipped to deal with
  such failures.

- The JavascriptTimelineMarker.h change, for the same reasons.

bz for pretty much everything, fitzgen for a second set of eyes and especially
to comment on the SavedStacks.cpp changes.
Attachment #8728447 - Flags: review?(nfitzgerald)
Attachment #8728447 - Flags: review?(bzbarsky)
Comment on attachment 8728447 [details] [diff] [review]
use |const char*| for representing async call reasons

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

Error handling in js/ LGTM for the most part. General rule of thumb: if the function (or method's class's ctor) takes a cx, then it will handle reporting OOM on the cx and you can just continue returning false/nullptr to signal failure. If it doesn't take a cx, then it is up to you to call ReportOutOfMemory(cx).

I have some big comments below about the scariness of lifetimes of these raw pointers we are passing around and storing on the JSRuntime, etc. I've convinced myself that all the existing uses are A-OK, but I'm still a little scared of future changes not being as careful as we are here, not realizing the care needed, or misunderstanding the lifetime requirements. I guess there isn't much we can do about it, except add some comments and hope people read them.

Overall: LGTM, r+ for js/ and SavedStacks parts! Thanks!

::: js/src/builtin/TestingFunctions.cpp
@@ +1111,5 @@
>      RootedObject stack(cx, &args[1].toObject());
>      RootedString asyncCause(cx, args[2].toString());
> +    JSAutoByteString utf8Cause;
> +    if (!utf8Cause.encodeUtf8(cx, asyncCause)) {
> +        JS_ReportError(cx, "Couldn't utf8 encode the async cause.");

I don't think you need to report an error on the cx, as encodeUtf8() takes a cx and the convention is that if a function takes a cx, then the function reports the error, not the caller.

You could MOZ_ASSERT(cx->isExceptionPending()) and the fuzzers should report back quickly enough whether conventions are being followed properly.

@@ +1118,2 @@
>  
> +    JS::AutoSetAsyncStackForNewCalls sas(cx, stack, utf8Cause.ptr(),

This is kind of scary wrt the lifetime of the generated string. The generated string has the lifetime of the JSAutoByteString RAII instance, and the runtime is going to store a pointer (without taking a copy/ownership).

But this is safe because the runtime gets that pointer unset by the `JS::AutoSetAsyncStackForNewCalls` RAII instance's dtor, and it happens before the `JSAutoByteString` destructs, right?

This is fairly subtle, and I am tempted to say that we should require that JS::AutoSetAsyncStackForNewCalls (and all the underlying async cause machinery) only deal with statically allocated strings for safety. However, AFAIK we can't enforce this on the type system level, so this would mostly be comments above `JS::AutoSetAsyncStackForNewCalls`. And I guess we have jit-tests tests relying on being able to change the async cause.

Perhaps that comment should just detail the lifetime constraints required and we can cross our fingers and hope everyone follows conventions correctly, as one usually does with C++ ;)

::: js/src/jsapi.h
@@ +4347,5 @@
>      // ambiguous whether that would clear any scheduled async stack and make the
>      // normal stack reappear in the new call, or just keep the async stack
>      // already scheduled for the new call, if any.
>      AutoSetAsyncStackForNewCalls(JSContext* cx, HandleObject stack,
> +                                 const char* asyncCause,

Lets add to the comment here about asyncCause's lifetime constraints (it must outlive the AutoSetAsyncStackForNewCalls lifetime) and how AutoSetAsyncStackForNewCalls and its underlying machinery does *NOT* take ownership of the asyncCause.

Probably with a recommendation to use statically allocated strings when possible.

::: js/src/vm/SavedStacks.cpp
@@ +1123,5 @@
> +                AutoCompartment ac(cx, iter.compartment());
> +                const char* cause = activation.asyncCause();
> +                UTF8Chars utf8Chars(cause, strlen(cause));
> +                size_t twoByteCharsLen = 0;
> +                char16_t* twoByteChars = UTF8CharsToNewTwoByteCharsZ(cx, utf8Chars,

Would it be problematic to make async cause a statically allocated `const char16_t*` instead of a `const char*`? Then we could avoid this conversion/allocation when capturing stacks.

Places where we set async cause would just need to `MOZ_UTF16("...")` instead of `"..."`.

@@ +1128,5 @@
> +                                                                     &twoByteCharsLen).get();
> +                if (!twoByteChars)
> +                    return false;
> +
> +                asyncCause = JS_NewUCString(cx, twoByteChars, twoByteCharsLen);

There is a relatively small set of async causes, and they will be reused many times (eg every async stack spanning a setTimeout will reuse the string "setTimeout handler" or whatever it is).

With that in mind, let's atomize the string here so we de-duplicate when possible.

  asyncCause = JS_AtomizeUCString(cx, twoByteChars);
  if (!asyncChars)
      return false;

::: js/xpconnect/src/XPCComponents.cpp
@@ +2712,2 @@
>  
> +    NS_ConvertUTF16toUTF8 utf8Cause(asyncCause);

Does this automatically free the string's chars on destruction?
Attachment #8728447 - Flags: review?(nfitzgerald) → review+
Thanks for the review!

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +1111,5 @@
> >      RootedObject stack(cx, &args[1].toObject());
> >      RootedString asyncCause(cx, args[2].toString());
> > +    JSAutoByteString utf8Cause;
> > +    if (!utf8Cause.encodeUtf8(cx, asyncCause)) {
> > +        JS_ReportError(cx, "Couldn't utf8 encode the async cause.");
> 
> I don't think you need to report an error on the cx, as encodeUtf8() takes a
> cx and the convention is that if a function takes a cx, then the function
> reports the error, not the caller.
> 
> You could MOZ_ASSERT(cx->isExceptionPending()) and the fuzzers should report
> back quickly enough whether conventions are being followed properly.

Ah, I was cargo-culting the error-handling patterns previous to this point and I see that I did it poorly.  Thanks for the explanation!

> @@ +1118,2 @@
> >  
> > +    JS::AutoSetAsyncStackForNewCalls sas(cx, stack, utf8Cause.ptr(),
> 
> This is kind of scary wrt the lifetime of the generated string. The
> generated string has the lifetime of the JSAutoByteString RAII instance, and
> the runtime is going to store a pointer (without taking a copy/ownership).
> 
> But this is safe because the runtime gets that pointer unset by the
> `JS::AutoSetAsyncStackForNewCalls` RAII instance's dtor, and it happens
> before the `JSAutoByteString` destructs, right?

Yes.

> ::: js/src/jsapi.h
> @@ +4347,5 @@
> >      // ambiguous whether that would clear any scheduled async stack and make the
> >      // normal stack reappear in the new call, or just keep the async stack
> >      // already scheduled for the new call, if any.
> >      AutoSetAsyncStackForNewCalls(JSContext* cx, HandleObject stack,
> > +                                 const char* asyncCause,
> 
> Lets add to the comment here about asyncCause's lifetime constraints (it
> must outlive the AutoSetAsyncStackForNewCalls lifetime) and how
> AutoSetAsyncStackForNewCalls and its underlying machinery does *NOT* take
> ownership of the asyncCause.
> 
> Probably with a recommendation to use statically allocated strings when
> possible.

Fair.

> ::: js/src/vm/SavedStacks.cpp
> @@ +1123,5 @@
> > +                AutoCompartment ac(cx, iter.compartment());
> > +                const char* cause = activation.asyncCause();
> > +                UTF8Chars utf8Chars(cause, strlen(cause));
> > +                size_t twoByteCharsLen = 0;
> > +                char16_t* twoByteChars = UTF8CharsToNewTwoByteCharsZ(cx, utf8Chars,
> 
> Would it be problematic to make async cause a statically allocated `const
> char16_t*` instead of a `const char*`? Then we could avoid this
> conversion/allocation when capturing stacks.
> 
> Places where we set async cause would just need to `MOZ_UTF16("...")`
> instead of `"..."`.

I looked into doing this because I didn't fully understand the JS engine's string functionality.  Doing this change winds up affecting a lot of areas that aren't directly related to this code for little benefit.

This might be useful as a followup.

> With that in mind, let's atomize the string here so we de-duplicate when
> possible.
> 
>   asyncCause = JS_AtomizeUCString(cx, twoByteChars);
>   if (!asyncChars)
>       return false;

I will do this with a comment summarizing the rationale.

> ::: js/xpconnect/src/XPCComponents.cpp
> @@ +2712,2 @@
> >  
> > +    NS_ConvertUTF16toUTF8 utf8Cause(asyncCause);
> 
> Does this automatically free the string's chars on destruction?

Well, the internal UTF8 buffer will be freed; it won't free |asyncCause|, but |asyncCause| is owned by the caller.  But we copy |utf8cause|'s characters into the JS engine when copying stacks anyway, so I don't think the lifetime of this string is a problem.
Comment on attachment 8728447 [details] [diff] [review]
use |const char*| for representing async call reasons

This looks pretty good in general. I have two nits:

1)  Please document the expected lifetime invariants in the places that take a const char* argument.

2)  Looking over this stuff, it seems like using a const uint16_t* argument, with MOZ_UTF16() in the places where we want literals, would allow skipping the NS_ConvertUTF16toUTF8 in XPCComponents and the UTF8CharsToNewTwoByteCharsZ in SavedStacks::insertFrames.  The main problem is that CallSetup's ctor takes a const char* execution reason, right?  We can fix that up, but followup is probably OK.
Attachment #8728447 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8728447 [details] [diff] [review]
> use |const char*| for representing async call reasons
> 
> This looks pretty good in general. I have two nits:
> 
> 1)  Please document the expected lifetime invariants in the places that take
> a const char* argument.

For avoidance of doubt, you mean not only AutoSetAsyncStackForNewCalls, as Nick suggested, but AutoEntryMonitor as well, and even NotifyJSRunToCompletionStart and JavascriptTimelineMarker?

> 2)  Looking over this stuff, it seems like using a const uint16_t* argument,
> with MOZ_UTF16() in the places where we want literals, would allow skipping
> the NS_ConvertUTF16toUTF8 in XPCComponents and the
> UTF8CharsToNewTwoByteCharsZ in SavedStacks::insertFrames.  The main problem
> is that CallSetup's ctor takes a const char* execution reason, right?  We
> can fix that up, but followup is probably OK.

Will file.
Flags: needinfo?(bzbarsky)
> For avoidance of doubt, you mean not only AutoSetAsyncStackForNewCalls, as Nick suggested,
> but AutoEntryMonitor as well, and even NotifyJSRunToCompletionStart and JavascriptTimelineMarker?

Anything that takes those const char* args.
Flags: needinfo?(bzbarsky)
(In reply to Wes Kocher (:KWierso) from comment #9)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/04ea7a18573d for a
> (possibly ASAN exclusive) leak:
> https://treeherder.mozilla.org/logviewer.html#?job_id=23699769&repo=mozilla-
> inbound

Yeah, for some reason I thought the allocated string was going to get freed on exit.  The fix is pretty straightforward, but I didn't have the resources to verify it live.
Flags: needinfo?(nfroyd)
Blocks: 1259736
https://treeherder.mozilla.org/logviewer.html#?job_id=24610576&repo=mozilla-inbound has
> PROCESS-CRASH | js/xpconnect/tests/chrome/test_bug732665.xul | application crashed [@ malloc_impl]
and is from a timeframe in which this patch hadn't been applied.
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #13)
> https://treeherder.mozilla.org/logviewer.html#?job_id=24610576&repo=mozilla-
> inbound has
> > PROCESS-CRASH | js/xpconnect/tests/chrome/test_bug732665.xul | application crashed [@ malloc_impl]
> and is from a timeframe in which this patch hadn't been applied.

Worth noting that was with PGO, not debug.  My patch may have tickled an intermittent, but it did make it a lot more reproducible, so...
Flags: needinfo?(nfroyd)
jandem made some changes to win64 stack space over in bug 1259699, and a try run seemed to show goodness:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8d8cddc85c5

I've therefore relanded the patch.
https://hg.mozilla.org/mozilla-central/rev/d4dce7faceac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This comment is about the landed code (also the patch, which is older though):

https://hg.mozilla.org/mozilla-central/rev/d4dce7faceac#l16.21

When I worked on async stacks performance, I remember SavedStacks::insertFrames was one of the places I had to look at. Have you considered the implications of allocating and atomizing a new string here at every stack capture? Since it's been quite some time ago and I'm working on other things now, I cannot look into this myself and maybe this change is just fine, but I wanted to call attention in case it's been overlooked.
(In reply to :Paolo Amadini from comment #18)
> This comment is about the landed code (also the patch, which is older
> though):
> 
> https://hg.mozilla.org/mozilla-central/rev/d4dce7faceac#l16.21
> 
> When I worked on async stacks performance, I remember
> SavedStacks::insertFrames was one of the places I had to look at. Have you
> considered the implications of allocating and atomizing a new string here at
> every stack capture? Since it's been quite some time ago and I'm working on
> other things now, I cannot look into this myself and maybe this change is
> just fine, but I wanted to call attention in case it's been overlooked.

Just to close the loop here: the problem that this bug was fixing is that we allocated JS strings everywhere where a stack *might* be captured.  Now we only allocate JS strings if a stack *is* captured, and we use atomization to try to avoid as many allocations as possible by sharing strings.

So if SavedStacks::insertFrame is now doing more work, that's appropriate, because the overhead should be at stack capture time.
You need to log in before you can comment on or make changes to this bug.