David Teller [:Yoric] - still alive but not very active Assignee
	Description • 11 years ago

As bug 903419, but for Promise.jsm.

I suspect we can implement this without too many difficulties using if we first write a C++ XPCOM component whose sole effect is to call Cu.reportError upon finalization.

Paolo, what do you think?

	Dave Townsend [:mossop]
	Comment 1 • 11 years ago

What I wouldn't give for an XPCOM component that can call a JS callback on destruction. I bet that causes crazy lifetime issues that I'm not thinking about though

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 2 • 11 years ago

Well, I can see how I would code it. I'm not sure it would work, of course :)

Anyway, for the sake of this task, we don't need to call a JS callback, just to call some C++ code and pass a string copied from JS during initialization.

	Dave Townsend [:mossop]
	Comment 3 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Well, I can see how I would code it. I'm not sure it would work, of course :)
> 
> Anyway, for the sake of this task, we don't need to call a JS callback, just
> to call some C++ code and pass a string copied from JS during initialization.

Yeah, I'm just dreaming of how wonderful it would be

	:Paolo Amadini
	Comment 4 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #0)
> I suspect we can implement this without too many difficulties using if we
> first write a C++ XPCOM component whose sole effect is to call
> Cu.reportError upon finalization.

This could work, though the error handling implementation should be smart enough
not to call into C++ or construct the component in the most common cases, to keep
the current level of performance.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 5 • 11 years ago

I don't have time to work on this bug at the moment, but if some brave soul wants to be mentored on that bug, I'm willing to mentor it.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 6 • 11 years ago

We can avoid any XPConnect cost by writing the finalization mechanism directly with JSAPI. The main cost that will remain will be capturing stack traces. Bug 724768 should remove most of that cost.

	Anup Kumar [:reznord]
	Comment 7 • 11 years ago

Hi, 

I am willing to work on this bug. So please I request you to assign it to me.

Thanks in Advance,

Regards,
A. Anup

	David Bruant
	Comment 8 • 11 years ago

(In reply to Anup Allamsetty from comment #7)
> I am willing to work on this bug. So please I request you to assign it to me.
> 
> Thanks in Advance,
My pleasure :-)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 9 • 11 years ago

Oh, well, I guess I'll take it.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 14 • 11 years ago

Here is a first prototype.
Note that the C++ code currently prints out uncaught errors to stderr.

	:Paolo Amadini
	Comment 17 • 11 years ago

The approach looks good to me. I just dislike ExnWatchdog as the component name, as I can't figure out
what it does from its name, and uses an unneeded/ambiguous abbreviation ("Exn" could as well mean
"extension" or "external"). ExceptionWatchdog is better but still imprecise, as it usually implies
a timer.

GCReporter, GCMessageReporter, GCErrorReporter, GCObserver, GCNotifier, FinalizationObserver,
FinalizationReporter seem better to me, there might even be better names.

Is it possible to have a global service exposing an addObserver() method to register a JavaScript
callback when reporting is needed? Or just use the observer notifications mechanism (for example an
"unhandled-promise-exception" notification)?

We don't convert the message to string because we might want to read the exception's stack. Are we
sure we can do that during the finalization that follows the garbage collection? Otherwise, we might
as well use a string argument when creating the reporter. If I understand correctly, the reporter
object is only created if "then" hasn't been called in the meantime, so this shouldn't be a big
overhead.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 18 • 11 years ago

(In reply to :Paolo Amadini from comment #17)
> The approach looks good to me. I just dislike ExnWatchdog as the component
> name, as I can't figure out
> what it does from its name, and uses an unneeded/ambiguous abbreviation
> ("Exn" could as well mean
> "extension" or "external"). ExceptionWatchdog is better but still imprecise,
> as it usually implies
> a timer.
> 
> GCReporter, GCMessageReporter, GCErrorReporter, GCObserver, GCNotifier,
> FinalizationObserver,
> FinalizationReporter seem better to me, there might even be better names.

Let's rename this to FinalizationWitness.

> Is it possible to have a global service exposing an addObserver() method to
> register a JavaScript
> callback when reporting is needed? Or just use the observer notifications
> mechanism (for example an
> "unhandled-promise-exception" notification)?

Interesting idea. This will complicate the C++ code a little but it will also make the feature more modular and easier to test.

> We don't convert the message to string because we might want to read the
> exception's stack. Are we
> sure we can do that during the finalization that follows the garbage
> collection? Otherwise, we might
> as well use a string argument when creating the reporter. If I understand
> correctly, the reporter
> object is only created if "then" hasn't been called in the meantime, so this
> shouldn't be a big
> overhead.

We definitely *cannot* convert to a string during finalization. The C++ code converts the exception to a string during construction (for the current prototype, it doesn't attempt to keep the stack, but that's easy to change).

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 11 years ago

Applied your feedback, made the mechanism more generic.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 21 • 11 years ago

Mike, I'm asking you for feedback, but don't hesitate to delegate to someone else.
Changes between v1 and v2: renamings.

	:Paolo Amadini
	Comment 22 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> Let's rename this to FinalizationWitness.

Looks good to me. I don't think I'll be able to do a full review on this in the next few days, but as soon as you think the interface is close to final, you may already request super-review to Dave to speed things up.

> We definitely *cannot* convert to a string during finalization. The C++ code
> converts the exception to a string during construction (for the current
> prototype, it doesn't attempt to keep the stack, but that's easy to change).

What could be interesting is reporting errors using Cu.reportError as if we were reporting a normal exception, that is with a hyperlink in the Console to the source code location where the error occurred.

We may store the required properties separately if we can only handle primitives, and construct an object resembling an exception before notifying the observers. Or if possible we could store a C++ XPCOM exception object that has been populated appropriately during the FinalizationWitness construction.

However, if that is too complicated, we could definitely convert the exception to its final message in JavaScript, and only report a single string to the observer.

	Mike Hommey [:glandium]
	Comment 23 • 11 years ago

Comment on attachment 800143 [details] [diff] [review]
Build and expose to xpcom, v2

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

::: toolkit/components/finalizationwitness/Makefile.in
@@ +6,5 @@
> +topsrcdir   = @top_srcdir@
> +srcdir      = @srcdir@
> +VPATH       = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

Remove the boilerplate. See https://groups.google.com/forum/#!topic/mozilla.dev.platform/EirpsT8JeYE

@@ +12,5 @@
> +LOCAL_INCLUDES += \
> +  -I$(topsrcdir)/js/xpconnect/loader \
> +  $(NULL)
> +
> +MOZILLA_INTERNAL_API = 1

LIBXUL_LIBRARY implies MOZILLA_INTERNAL_API, so you can remove it.

@@ +14,5 @@
> +  $(NULL)
> +
> +MOZILLA_INTERNAL_API = 1
> +
> +include $(topsrcdir)/config/rules.mk

That include can go too.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 24 • 11 years ago

Comment on attachment 800141 [details] [diff] [review]
JSAPI code, v2

I just looked at the GC aspects, but it seems safe to do this.

	Jeff Walden [:Waldo]
	Comment 25 • 11 years ago

Comment on attachment 800141 [details] [diff] [review]
JSAPI code, v2

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

I assume the idea is a Promise will close over a FinalizationWitness |fw| created with suitable documentation strings of some sort?  And "using" a Promise will call |fw.forget()|?  You might want to consider having this behavior pref-controlled, because depending on promise-traffic this might be a bit of extra GC pressure for no good except to people doing debugging.

::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp
@@ +4,5 @@
> +
> +#include "FinalizationWitnessService.h"
> +
> +#include "nsString.h"
> +#include "jspubtd.h"

You're using JSAPI methods, so you need jsapi.h -- you're just bootlegging and kidding yourself if you think jspubtd.h is effective here.

@@ +7,5 @@
> +#include "nsString.h"
> +#include "jspubtd.h"
> +#include "mozJSComponentLoader.h"
> +
> +#include "mozilla/Scoped.h"

I'd be happier with a mozilla/NullPtr.h here as well.

@@ +80,5 @@
> +  }
> +
> +  // Notify observers. Since we are executed during garbage-collection,
> +  // we need to dispatch the notification to the main thread.
> +  nsCOMPtr<nsIRunnable> event = new FinalizationEvent(privateData.forget());

Not that it likely matters in terms of efficiency, etc.  But why not combine the FinalizationEvent and PrivateData into a single class?  Save an allocation, simplify tracking of lifetimes by eliminating a thing to think about, etc.

@@ +89,5 @@
> +  JS_SetPrivate(objSelf, nullptr);
> +  return;
> +}
> +
> +static JSClass sWatchdogClass = {

This'll need to be const, now.

@@ +91,5 @@
> +}
> +
> +static JSClass sWatchdogClass = {
> +  "FinalizationWitness",
> +  JSCLASS_HAS_PRIVATE,

JSCLASS_HAS_PRIVATE is in the process of being deprecated and removed.  Use JSCLASS_HAS_RESERVED_SLOTS(1) instead, and store/access the slot as a PrivateValue (see https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/jsval for details).  (Under the hood we desugar JSCLASS_HAS_PRIVATE into an extra slot in the object, so the reserved slot approach is functionally identical to using JSCLASS_HAS_PRIVATE.)

@@ +110,5 @@
> + *
> + *  Neutralize the watchdog. Once this method is called, the watchdog will
> + *  never report any error.
> + */
> +bool Forget(JSContext *cx, unsigned argc, jsval *vp)

JS::Value

@@ +117,5 @@
> +  if (args.length() != 0) {
> +    JS_ReportError(cx, "forget() takes no arguments");
> +    return false;
> +  }
> +  JS::Rooted<JSObject*> objSelf(cx, args.thisv().toObjectOrNull());

This'll assert if someone does:

  witness.forget.call(17);

because args.thisv() is the exact value passed in by the caller.

What you should do instead is use JS::CallNonGenericMethod, which will guarantee |this| is exactly the sort of value you want it to be, used with a method like so:

static bool
IsWatchDog(JS::Handle<JS::Value> v)
{
  return v.isObject() && JS_GetClass(&v.toObject()) == &sWatchdogClass;
}

This also means you can just use

  JS::Rooted<JSObject*> objSelf(cx, &args.thisv().toObject());

and

  static_cast<PrivateData*>(JS_GetPrivate(objSelf)) rather than a bulky JS_GetInstancePrivate call.  (I think that call might have already complained, actually, if called on a wrong-class object (think fw.forget.call({})) -- yet another reason to use the does-exactly-one-thing JS_GetPrivate method instead.)

@@ +130,5 @@
> +    JS_ReportError(cx, "forget() called twice");
> +    return false;
> +  }
> +
> +  privateData.dispose();

Why dispose explicitly if you're going to use the scoped-delete class?

@@ +133,5 @@
> +
> +  privateData.dispose();
> +
> +  JS_SetPrivate(objSelf, nullptr);
> +  return true;

Failure to do args.rval().set(...) or one of the setFoo typed variants means the native returns an unspecified value.  You need |args.rval().setUndefined();| to get the behavior you want.

@@ +138,5 @@
> +}
> +
> +
> +static const JSFunctionSpec sWatchdogClassFunctions[] = {
> +  JS_FN("forget", Forget, 0, (JSPROP_READONLY | JSPROP_PERMANENT)),

Redundant parentheses.

@@ +155,5 @@
> + * watchdog is created, call its method |forget()| to prevent the
> + * exception from being reported.
> + *
> + * @param aError The error captured by the watchdog. It may be any
> + *               JavaScript value, including |undefined|.

This argument-documentation looks wrong.

@@ +180,5 @@
> +  JS::Rooted<JSString*> strTopic(cx, JS_ValueToString(cx, args[0]));
> +  if (!strTopic) {
> +    return false;
> +  }
> +  const jschar* charpTopic = JS_GetStringCharsZ(cx, strTopic);

If you're going to do this, for now (until we have generational GC and exact rooting working) you need a JS::Anchor<JSString*>(strTopic).  Otherwise the compiler's free to treat |strTopic| as unused, reuse that register, and trick SpiderMonkey's conservative stack scanner into thinking |strTopic| is free to be garbage-collected.

@@ +190,5 @@
> +  JS::Rooted<JSString*> strException(cx, JS_ValueToString(cx, args[1]));
> +  if (!strException) {
> +    return false;
> +  }
> +  const jschar* charpException = JS_GetStringCharsZ(cx, strException);

Mutatis mutandis here.

@@ +197,5 @@
> +  }
> +
> +  ScopedDeletePtr<PrivateData> privateData(new PrivateData(charpTopic,
> +                                                           charpException));
> +  JS_SetPrivate(objResult, privateData);

Why not privateData.forget() here?  Or even forget the ScopedDeletePtr entirely, because it's obviously pointless when you're using up the value on the immediate next line?

@@ +199,5 @@
> +  ScopedDeletePtr<PrivateData> privateData(new PrivateData(charpTopic,
> +                                                           charpException));
> +  JS_SetPrivate(objResult, privateData);
> +
> +  JS::Rooted<jsval> valResult(cx, OBJECT_TO_JSVAL(objResult));

JS::Value instead of jsval, jsval's deprecated now.  (Also you should use JS::ObjectValue and friends instead of *_TO_JSVAL, same reason.  But I don't think you need either, in this case, because below.)

@@ +201,5 @@
> +  JS_SetPrivate(objResult, privateData);
> +
> +  JS::Rooted<jsval> valResult(cx, OBJECT_TO_JSVAL(objResult));
> +
> +  JS_SET_RVAL(cx, vp, valResult);

args.rval().setObject(*objResult), JS_SET_RVAL is deprecated.

@@ +209,5 @@
> +
> +
> +static JSClass sWatchdogConstructor = {
> +  "FinalizationWitness",
> +  JSCLASS_HAS_PRIVATE,

This wants a reserved slot as well, same reasons.

@@ +226,5 @@
> +};
> +
> +bool DefineFinalizationWitness(JSContext *cx, JSObject *aObject)
> +{
> +  return !!JS_DefineObject(cx, aObject, "FinalizationWitness",

No need for !!.

::: toolkit/components/finalizationwitness/FinalizationWitnessService.h
@@ +9,5 @@
> +
> +/**
> + * XPConnect initializer, for use in the main thread.
> + */
> +class FinalizationWitnessService MOZ_FINAL : public nsIFinalizationWitnessService

I'd prefer if you #included mozilla/Attributes.h directly for this, rather than relying on bootlegging.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 26 • 11 years ago

Thanks for the review, I'll see what I can do about each point.
Note: Many of the things that you suggest do not appear in the JSAPI reference, or at least not in a form that would let me find them. I suspect that the reference will need lots of updating.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 28 • 11 years ago

Note to self: according to my micro-benchmarks (which may or may not be valid), we should delay accessing the |stack| property of the exception as long as we can, and rather store the error in a global map until we are certain that the stack information is needed. Even if we don't include string copies, this seems much more efficient.

	Jeff Walden [:Waldo]
	Comment 29 • 11 years ago

Comment on attachment 806563 [details] [diff] [review]
JSAPI code, v3

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

Mostly nitpicks, looks generally fine.  But the refcounting stuff is just asking for a crash or a leak or whatever if it's wrong, so I'm going to have to see those bits again before I'll r+.

::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp
@@ +32,5 @@
> +{
> +public:
> +FinalizationEvent(const jschar* aTopic, const jschar *aMessage):
> +  mTopic(aTopic),
> +  mMessage(aMessage) {

This looks like some really broken indentation.

@@ +83,5 @@
> +    // Forget() has been called
> +    return;
> +  }
> +
> +  ScopedDeletePtr<FinalizationEvent> event(static_cast<FinalizationEvent*>(JSVAL_TO_PRIVATE(slotEvent)));

slotEvent.toPrivate()

@@ +87,5 @@
> +  ScopedDeletePtr<FinalizationEvent> event(static_cast<FinalizationEvent*>(JSVAL_TO_PRIVATE(slotEvent)));
> +
> +  // Notify observers. Since we are executed during garbage-collection,
> +  // we need to dispatch the notification to the main thread.
> +  (void)NS_DispatchToMainThread(event.forget());

Wait a second.  ScopedDeletePtr makes basically no sense.  FinalizationEvent is XPCOM and reference-counted.  We shouldn't go behind the reference-count's back here, for our own sanity.  Also it's pretty much a violation of XPCOM rules to pass a zero-refcount newborn to another method -- the caller has to keep a reference, the callee should be able to addref/release as much as it wants as long as it does so balanced-ly.  (Things appear to work out here in practice.  But it's a pretty fragile/dangerous state to be in.)

So instead, make this (in concert with the ref-counting changes requested later on -- there's some tricky leakiness here if you don't do both):

  nsRefPtr<FinalizationEvent> event = already_AddRefed(static_cast<FinalizationEvent*>(slotEvent.toPrivate()));
  NS_DispatchToMainThread(event);

@@ +92,5 @@
> +  // We may fail at dispatching to the main thread if we arrive too late
> +  // during shutdown. In that case, there is not much we can do.
> +
> +  JS_SetReservedSlot(objSelf, WITNESS_SLOT_EVENT, JS::UndefinedValue());
> +  return;

Not sure why the final return here...

@@ +129,5 @@
> +    JS_ReportError(cx, "forget() takes no arguments");
> +    return false;
> +  }
> +  JS::Rooted<JS::Value> valSelf(cx, args.thisv());
> +  if (!IsWitness(valSelf)) {

This isn't quite what I meant.  Instead have:

bool IsWitness(...) { ... }

bool ForgetImpl(JSContext* cx, JS::CallArgs args)
{
  if (args.length() != 0) { ... }

  JS::Rooted<JSObject*> objSelf(cx, &args.thisv().toObject());
  ...
}

bool Forget(JSContext* cx, unsigned argc, JS::Value* vp)
{
  JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
  return JS::CallNonGenericMethod<IsWitness, ForgetImpl>(cx, args);
}

Or thereabouts, I scrawled this from memory.

JS::CallNonGenericMethod will do all the work to make sure that the method, if invoked, will only work if given a correct |this| -- including working if passed a witness from another global object and all.  All you do is provide the predicate to say that a value is an acceptable |this| for the implementation function to work with.

@@ +134,5 @@
> +    JS_ReportError(cx, "forget() can only be called on a FinalizationWitness");
> +    return false;
> +  }
> +
> +  JS::Rooted<JSObject*> objSelf(cx, valSelf.toObjectOrNull());

&val.toObject() is preferable, because it makes explicit that the initializing value is always non-null.

@@ +142,5 @@
> +    JS_ReportError(cx, "forget() called twice");
> +    return false;
> +  }
> +
> +  ScopedDeletePtr<FinalizationEvent> event(static_cast<FinalizationEvent*>(JSVAL_TO_PRIVATE(slotEvent)));

There are a few places for this, so let's extract this get-slot-then-private-then-cast pattern into a single method and use that everywhere:

static already_AddRefed<FinalizationEvent>
ExtractFinalizationEvent(JSContext* cx, JS::Handle<JSObject*> obj)
{
  JS::Rooted<JS::Value> slot(cx, JS_GetReservedSlot(obj, WITNESS_SLOT_EVENT));
  if (slot.isUndefined())
    return nullptr;
  already_AddRefed<FinalizationEvent> f = already_AddRefed(static_cast<FinalizationEvent*>(slot.toPrivate()));
  JS_SetReservedSlot(obj, WITNESS_SLOT_EVENT, JS::UndefinedValue());
  return f;
}

Then the close of this method can be:

  nsRefPtr<FinalizationEvent> event = ExtractFinalizationEvent(cx, objSelf);
  if (!event) {
    JS_ReportError(cx, "forget() called twice");
    return false;
  }
  args.rval().setUndefined();
  return true;

@@ +193,5 @@
> +  if (!strTopic) {
> +    return false;
> +  }
> +  JS::Anchor<JSString*> anchorTopic(strTopic);
> +  const jschar* charpTopic = JS_GetStringCharsZ(cx, strTopic);

I should have mentioned this before, but could you use JS_GetStringCharsAndLength instead here, then have FinalizationEvent be initialized using those lengths?  It's better/more efficient to not be recomputing string lengths.  Plus, since JS strings can easily have embedded nulls, it brings us closer to (or at least doesn't take us further away from) getting rid of null-termination as a concept at all for such strings.

@@ +212,5 @@
> +  }
> +
> +  FinalizationEvent* event = new FinalizationEvent(charpTopic,
> +                                                   charpValue);
> +  JS_SetReservedSlot(objResult, WITNESS_SLOT_EVENT, PRIVATE_TO_JSVAL(event));

Hmm.  Storing an object with a refcount of zero like this seems like not the happiest-making idea.  How about you assign into an nsRefPtr, then use PrivateValue(event.forget()) here?

@@ +236,5 @@
> +  nullptr /* call */,
> +  Construct /* construct */,
> +};
> +
> +bool DefineFinalizationWitness(JSContext *cx, JSObject *aObject)

If this method is only called one place, why not just put its contents inline in FinalizationWitnessService::Init?

@@ +261,5 @@
> +
> +NS_IMETHODIMP
> +FinalizationWitnessService::Init(JSContext *aCx)
> +{
> +  mozJSComponentLoader* loader = mozJSComponentLoader::Get();

Should there be a |if (!loader) return NS_ERROR_NOT_AVAILABLE;|?

::: toolkit/components/finalizationwitness/FinalizationWitnessService.h
@@ +16,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIFINALIZATIONWITNESSSERVICE
> +  FinalizationWitnessService();
> +private:
> +  ~FinalizationWitnessService();

Why have constructor/destructor declared at all if they're going to be empty?  Seems better to just rely on the defaults.  (That, and perhaps delete the copy constructor and assignment operator, if doing so doesn't disable implicit default-ctor/dtor generation.)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 30 • 11 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #29)
> Wait a second.  ScopedDeletePtr makes basically no sense.

Oops! Silly me.

>   nsRefPtr<FinalizationEvent> event =
> already_AddRefed(static_cast<FinalizationEvent*>(slotEvent.toPrivate()));
>   NS_DispatchToMainThread(event);

I suspect that you meant |dontAddRef|, don't you?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 31 • 11 years ago

Code is now a little more complex but we now capture the stack at (almost) no cost.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 32 • 11 years ago

Applied feedback.

	:Paolo Amadini
	Comment 33 • 11 years ago

Comment on attachment 807690 [details] [diff] [review]
JS code, v3

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

This is a preliminary first pass, just some general observations and nitpicking for the moment :-)

I like the approach here and how we only store errors for later if a promise is rejected and we don't have a handler yet.

In most cases, handlers are registered before the callbacks run so this should only have an impact if errors aren't properly handled already, or in rare edge cases.

::: toolkit/modules/Promise.jsm
@@ -94,5 @@
>  
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
> -const Cr = Components.results;

Even if unused, I think we should keep this definition. It's far too common to copy-and-paste code only to find out it doesn't work because one of these four definitions are missing.

@@ +126,5 @@
> +// Additionally, we notify "promise-finalization-witness" so as to let
> +// additional clients (e.g. Devtools) inform developers of ongoing issues.
> +//
> +// Note that this mechanism can cause some rejections to be reported
> +// multiple times.

We only report rejections that cannot be reported anymore because the promise object has no more accessible references, right? So, this shouldn't result in multiple reports of the same error.

@@ +127,5 @@
> +// additional clients (e.g. Devtools) inform developers of ongoing issues.
> +//
> +// Note that this mechanism can cause some rejections to be reported
> +// multiple times.
> +const TOPIC_FINALIZATION = "promise-finalization-witness";

This should more properly be called TOPIC_PROMISE_FINALIZATION. Actually, we can just use the string "promise-finalization" in the two instances where this is used, and update the comment above. A constant doesn't add much, though it helps in missing references in comments when doing full-text searches :-)

@@ +143,5 @@
> + *
> + * @constructor
> + */
> +Cc["@mozilla.org/toolkit/finalizationwitness;1"].
> + getService(Ci.nsIFinalizationWitnessService).init();

I think any initialization should happen automatically when the first FinalizationWitness is created. I've not looked at the C++ code, but I guess there is a way to do that.

@@ +162,5 @@
> +  *
> +  */
> + register: function(error) {
> +   let id = "pending-error-" + (this._counter++);
> +   this._map.set(id, error);

My concern here is that the error object can be pretty much anything, and if it references even indirectly or through closures the promise object we've started from, this will cause a memory leak in addition to the error not being reported.

We should probably copy the relevant error properties and build our own error object for reporting. After all, the full reporting can be done by the rejection handler, this is a just a fallback mechanism.

@@ +179,5 @@
> +// Actually print the finalization warning.
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  Components.utils.reportError("A promise chain failed to handle a rejection.");
> +  let error = PendingErrors.extract(aValue);
> +  Components.utils.reportError(error);

We should make it clear that the error happened at an earlier time (actually, now that I think about it, capturing and reporting the time when the error occurred can be useful).

@@ +539,5 @@
> +   * When the N_STATUS property is STATUS_REJECTED and until there is
> +   * a rejection callback, this contains an instance of FinalizationWitness
> +   * capturing the error that has been passed to the promise.
> +   */
> +  Object.defineProperty(this, N_WITNESS, { writable: true });

nit: blank line

@@ +669,5 @@
> +        /*nextStatus == STATUS_REJECTED*/
> +        if (typeof(this.onReject) == "function") {
> +          nextValue = this.onReject(nextValue);
> +          nextStatus = STATUS_RESOLVED;
> +        }

This change doesn't really enhance readability, you can either revert or improve it.

::: toolkit/modules/tests/xpcshell/test_Promise.js
@@ +734,5 @@
> +      Services.console.unregisterListener(observer);
> +      observer = null;
> +    }
> +    deferred.reject(new Error("Timeout"));
> +  });

We shouldn't have our own shorter timeout here, lest we cause intermittent failures. We should only wait for the message if it is expected, and if the message doesn't arrive, the entire test will time out.

I haven't reviewed the other tests in detail yet.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 34 • 11 years ago

(In reply to :Paolo Amadini from comment #33)
> @@ +126,5 @@
> > +// Additionally, we notify "promise-finalization-witness" so as to let
> > +// additional clients (e.g. Devtools) inform developers of ongoing issues.
> > +//
> > +// Note that this mechanism can cause some rejections to be reported
> > +// multiple times.
> 
> We only report rejections that cannot be reported anymore because the
> promise object has no more accessible references, right? So, this shouldn't
> result in multiple reports of the same error.

I believe that we can have multiple reports in the following scenario:

let deferred = Promise.defer();
let p1 = deferred.promise.then();
let p2 = deferred.promise.then();
deferred.reject("Boom");
deferred = null;
p1 = null;
p2 = null;
// Error is unhandled both in p1 and in p2.

> @@ +143,5 @@
> > + *
> > + * @constructor
> > + */
> > +Cc["@mozilla.org/toolkit/finalizationwitness;1"].
> > + getService(Ci.nsIFinalizationWitnessService).init();
> 
> I think any initialization should happen automatically when the first
> FinalizationWitness is created. I've not looked at the C++ code, but I guess
> there is a way to do that.

Well, this call to |init()| just creates toplevel constructor |FinalizationWitness|, at a cost close to 0. I can hide this behind a lazy getter, but this will complicate things a little.


> @@ +162,5 @@
> > +  *
> > +  */
> > + register: function(error) {
> > +   let id = "pending-error-" + (this._counter++);
> > +   this._map.set(id, error);
> 
> My concern here is that the error object can be pretty much anything, and if
> it references even indirectly or through closures the promise object we've
> started from, this will cause a memory leak in addition to the error not
> being reported.
> 
> We should probably copy the relevant error properties and build our own
> error object for reporting. After all, the full reporting can be done by the
> rejection handler, this is a just a fallback mechanism.

Indeed, there will be a cycle and no warning if the error object references to the promise object.
Note that, if we copy the relevant error properties at registration time, the whole register-to-avoid-calling-.stack mechanism becomes rather useless, because we have called .stack in the first place. Is that ok with you?

> ::: toolkit/modules/tests/xpcshell/test_Promise.js
> @@ +734,5 @@
> > +      Services.console.unregisterListener(observer);
> > +      observer = null;
> > +    }
> > +    deferred.reject(new Error("Timeout"));
> > +  });
> 
> We shouldn't have our own shorter timeout here, lest we cause intermittent
> failures. We should only wait for the message if it is expected, and if the
> message doesn't arrive, the entire test will time out.
> 
> I haven't reviewed the other tests in detail yet.

Well, when we test the symmetric property ("don't display the error if it is caught"), we need a timeout, so I figured we should have symmetric tests.

	Jeff Walden [:Waldo]
	Comment 35 • 11 years ago

.stack's computation on Error objects is weird right now.  It purports to be lazy, and it might save a little time at init time, but really it does about twice the effort it needs to in the course of being lazy.  (It creates this big huge tracking data structure at init time, then it parses and generates a string when .stack is read -- every time, even, if it's accessed multiple times.)

I have partial patchwork to make .stack cut out the middle man and just create the stack string right off the bat.  Sometime soon, all the work to compute .stack's data will be done at creation time, not at access time.  So I'd say don't assume doing extra work initially, can avoid the cost of computing stack.

	Jeff Walden [:Waldo]
	Comment 36 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #30)
> >   nsRefPtr<FinalizationEvent> event =
> > already_AddRefed(static_cast<FinalizationEvent*>(slotEvent.toPrivate()));
> >   NS_DispatchToMainThread(event);
> 
> I suspect that you meant |dontAddRef|, don't you?

I...don't think so?

http://mxr.mozilla.org/mozilla-central/search?string=dontAddRef

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 11 years ago

There's dont_AddRef, but it's exactly the same thing as already_AddRefed.

	:Paolo Amadini
	Comment 38 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #34)
> (In reply to :Paolo Amadini from comment #33)
> > @@ +126,5 @@
> > > +// Additionally, we notify "promise-finalization-witness" so as to let
> > > +// additional clients (e.g. Devtools) inform developers of ongoing issues.
> > > +//
> > > +// Note that this mechanism can cause some rejections to be reported
> > > +// multiple times.
> > 
> > We only report rejections that cannot be reported anymore because the
> > promise object has no more accessible references, right? So, this shouldn't
> > result in multiple reports of the same error.
> 
> I believe that we can have multiple reports in the following scenario:

I see what you mean, it makes sense and should be explained in the comments.

> Well, this call to |init()| just creates toplevel constructor
> |FinalizationWitness|, at a cost close to 0. I can hide this behind a lazy
> getter, but this will complicate things a little.

That's not clear from the method's name, maybe the method should be renamed to something along the lines of importFinalizationWitnessConstructor. It looks weird anyways. Isn't there an alternative technique that can be used, like a real XPCOM constructor or an nsIFinalizationWitnessService.create method?

These are my initial thoughts, the interface will need super-review in any case.

> Indeed, there will be a cycle and no warning if the error object references
> to the promise object.
> Note that, if we copy the relevant error properties at registration time,
> the whole register-to-avoid-calling-.stack mechanism becomes rather useless,
> because we have called .stack in the first place. Is that ok with you?

Per Jeff's comment, we should definitely create the error properties earlier.

> Well, when we test the symmetric property ("don't display the error if it is
> caught"), we need a timeout, so I figured we should have symmetric tests.

A failure to process within the timeout can't cause an intermittent failure here, so it's very different than the other case.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 39 • 11 years ago

(In reply to :Paolo Amadini from comment #38)
> I see what you mean, it makes sense and should be explained in the comments.

Will do.

> That's not clear from the method's name, maybe the method should be renamed
> to something along the lines of importFinalizationWitnessConstructor. It
> looks weird anyways. Isn't there an alternative technique that can be used,
> like a real XPCOM constructor or an nsIFinalizationWitnessService.create
> method?
> 
> These are my initial thoughts, the interface will need super-review in any
> case.

Well, this is more or less standard procedure for injecting a JSAPI object. I can rename |init| to |inject| or something along these lines, but doing anything more sophisticated will complicate the C++ code without simplifying the JS code.

> > Well, when we test the symmetric property ("don't display the error if it is
> > caught"), we need a timeout, so I figured we should have symmetric tests.
> 
> A failure to process within the timeout can't cause an intermittent failure
> here, so it's very different than the other case.

ok, let's do that

	Jeff Walden [:Waldo]
	Comment 40 • 11 years ago

Comment on attachment 807692 [details] [diff] [review]
JSAPI code, v4

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

::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp
@@ +35,5 @@
> +{
> +public:
> +FinalizationEvent(const jschar* aTopic,
> +                  const size_t aLenTopic,
> +                  const jschar *aValue,

Keep the * consistent here.  I'd remove the const on size_t, too, and make it aTopicLen/aValueLen if it were me.

@@ +39,5 @@
> +                  const jschar *aValue,
> +                  const size_t aLenValue)
> +  : mTopic(aTopic, aLenTopic)
> +  , mValue(aValue, aLenValue)
> +  { }

This constructor indentation is still bizarre.

@@ +91,5 @@
> +  }
> +
> +  JS_SetReservedSlot(objSelf, WITNESS_SLOT_EVENT, JS::UndefinedValue());
> +
> +  return dont_AddRef(static_cast<FinalizationEvent*>(slotEvent.toPrivate()));

Oh, dont_AddRef.  With body identical to an already_AddRefed cast.  Works for me, plus gets rid of FinalizationEvent repetition.

@@ +164,5 @@
> +
> +bool Forget(JSContext *cx, unsigned argc, JS::Value *vp)
> +{
> +  JS::CallArgs args = CallArgsFromVp(argc, vp);
> +  return JS::CallNonGenericMethod<IsWitness, ForgetImpl>(cx, args);

Beware: we might still use compilers that can't handle using a value with (without?  can't remember which) linkage as a template parameter, only legal since C++11.  If you hit this, you'll need to move IsWitness and ForgetImpl outside the anonymous namespace.

@@ +211,5 @@
> +    return false;
> +  }
> +  JS::Anchor<JSString*> anchorTopic(strTopic);
> +  size_t lenTopic = 0;
> +  const jschar* charpTopic = JS_GetStringCharsAndLength(cx, strTopic, &lenTopic);

I'd have to think just |topic| and |topicLen| would be more common names than this.

@@ +233,5 @@
> +                                                            lenTopic,
> +                                                            charpValue,
> +                                                            lenValue);
> +  JS_SetReservedSlot(objResult, WITNESS_SLOT_EVENT, PRIVATE_TO_JSVAL(event));
> +  event.forget(); // Ensure that the slot is already addrefed

JS::PrivateValue(event.forget().get()), and get rid of the second forget(), of course.  (I don't think you need a comment here -- forgetting the refptr into a reserved slot fairly obviously preserves/transfers the addref.)

@@ +284,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

Extra (?) trailing blank line.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 41 • 11 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40)
> @@ +39,5 @@
> > +                  const jschar *aValue,
> > +                  const size_t aLenValue)
> > +  : mTopic(aTopic, aLenTopic)
> > +  , mValue(aValue, aLenValue)
> > +  { }
> 
> This constructor indentation is still bizarre.

What did you expect?

> @@ +164,5 @@
> > +
> > +bool Forget(JSContext *cx, unsigned argc, JS::Value *vp)
> > +{
> > +  JS::CallArgs args = CallArgsFromVp(argc, vp);
> > +  return JS::CallNonGenericMethod<IsWitness, ForgetImpl>(cx, args);
> 
> Beware: we might still use compilers that can't handle using a value with
> (without?  can't remember which) linkage as a template parameter, only legal
> since C++11.  If you hit this, you'll need to move IsWitness and ForgetImpl
> outside the anonymous namespace.

Ok, I'll do that if TryServer or someone complains.

 
> @@ +211,5 @@
> > +    return false;
> > +  }
> > +  JS::Anchor<JSString*> anchorTopic(strTopic);
> > +  size_t lenTopic = 0;
> > +  const jschar* charpTopic = JS_GetStringCharsAndLength(cx, strTopic, &lenTopic);
> 
> I'd have to think just |topic| and |topicLen| would be more common names
> than this.

Well, we have the topic as a JSSstring, as a jschar* and as a size_t, so I went for hungarianish notation to avoid confusing them.
 
> JS::PrivateValue(event.forget().get()), and get rid of the second forget(),
> of course.  (I don't think you need a comment here -- forgetting the refptr
> into a reserved slot fairly obviously preserves/transfers the addref.)

I have had to read code that did this kind of manipulations and this wasn't always understandable without comments, so I preferred to err on the side of slight over-commenting.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 42 • 11 years ago

Applied feedback, carrying over r+.
Dave, can you sr? this component?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 43 • 11 years ago

Applied feedback. I took the opportunity to add a few more tests.

	:Paolo Amadini
	Comment 44 • 11 years ago

Comment on attachment 809773 [details] [diff] [review]
JS Code, v4

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

The new feature explanations look great! :-) Notes below.

::: toolkit/modules/Promise.jsm
@@ +156,5 @@
> + * when it is garbage-collected. Once the watchdog is created, call
> + * method its method |forget()| to prevent the broadcast.
> + *
> + * @param aString The value that the watchdog will broadcast. It will
> + *                be converted to a string.

Document both parameters of the FinalizationWitness constructor.

@@ +165,5 @@
> +  get: function() {
> +    delete this.FinalizationWitness;
> +    // Inject FinalizationWitness from native code as a global object.
> +    Cc["@mozilla.org/toolkit/finalizationwitness;1"].
> +      getService(Ci.nsIFinalizationWitnessService).inject();

This "inject" method still looks strange to me, but I trust the C++ review and super-review for that.

@@ +180,5 @@
> +  forget: function() {
> +    ...
> +  }
> +};
> +*/

Please move this to the constructor documentation, indented with asterisks so as not to mistake this for active code.

@@ +183,5 @@
> +};
> +*/
> +
> +let PendingErrors = {
> + _counter: 0,

Two space intentation please.

@@ +240,5 @@
> +  Components.utils.reportError(description);
> +  Components.utils.reportError("Date: " + date);
> +  if (stack) {
> +    Components.utils.reportError("Full stack: " + stack + "\n");
> +  }

I think we should make this a single entry, so that is more easily copied to clipboard, and avoid any concurrency issue with other errors.

I also think we should make the lineNumber and fileName reported as error meta-data, so that the original location of the error can be opened from the Console (instead of the Promise.jsm file).

@@ +596,5 @@
>  
> +  /**
> +   * When the N_STATUS property is STATUS_REJECTED and until there is
> +   * a rejection callback, this contains an instance of FinalizationWitness
> +   * capturing the error that has been passed to the promise.

Update the comment to reflect the actual contents of the property.

::: toolkit/modules/tests/xpcshell/xpcshell.ini
@@ +14,5 @@
>  [test_sqlite.js]
>  [test_task.js]
>  [test_TelemetryTimestamps.js]
>  [test_timer.js]
> +[test_Promise.js]

Already present.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 45 • 11 years ago

Applied feedback.
Also, we now report the error through Services.console instead of Cu.reportError.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 46 • 11 years ago

Comment on attachment 809770 [details] [diff] [review]
JSAPI code, v5

Oops, I obsoleted the wrong attachment.

	Dave Townsend [:mossop]
	Comment 47 • 11 years ago

Comment on attachment 809770 [details] [diff] [review]
JSAPI code, v5

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

I'd like to see answers to the questions here before signing off.

::: toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl
@@ +18,5 @@
> +   * Create a new Finalization Witness.
> +   *
> +   * A finalization witness is an object whose sole role is to
> +   * broadcast when it is garbage-collected. Once the witness is
> +   * created, call method its method |forget()| to prevent the

one too many method here

@@ +22,5 @@
> +   * created, call method its method |forget()| to prevent the
> +   * broadcast.
> +   *
> +   * @param aTopic The topic that the witness will broadcast using
> +   *               Services.obs.

"using the observer service"

@@ +26,5 @@
> +   *               Services.obs.
> +   * @param aString The string that the witness will broadcast.
> +   *
> +   * @constructor
> +   * function FinalizationWitness(aTopic, aString) {

I suppose there are horrible issues if we also allowed a subject for the observer notification to be included here?

@@ +39,5 @@
> +   *   }
> +   *};
> +   */
> +  [implicit_jscontext]
> +  void inject();

This is a bit of an odd API. Is there a reason that we can't just have a createWitness method that returns a witness?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 48 • 11 years ago

(In reply to Dave Townsend (:Mossop) from comment #47)
> @@ +26,5 @@
> > +   *               Services.obs.
> > +   * @param aString The string that the witness will broadcast.
> > +   *
> > +   * @constructor
> > +   * function FinalizationWitness(aTopic, aString) {
> 
> I suppose there are horrible issues if we also allowed a subject for the
> observer notification to be included here?

Since the subject is a nsISupports, that could complicate the gc semantics. This might be possible, but if this is ok, I'd rather not include this in v1.

> 
> @@ +39,5 @@
> > +   *   }
> > +   *};
> > +   */
> > +  [implicit_jscontext]
> > +  void inject();
> 
> This is a bit of an odd API. Is there a reason that we can't just have a
> createWitness method that returns a witness?

I have tried to involved XPConnect as little as possible in the process, to be on the safe side wrt gc semantics, as well as to avoid unneeded string conversions. Constructor FinalizationWitness is therefore not implemented in XPCOM/XPConnect but in JSAPI. This kind of injection is basically how JSAPI objects are exported to JS, see e.g. ctypes.jsm, nsIOSFileConstants.idl.

Now, I suppose I could investigate reimplementing this as a method |jsval createWitness(string, string)|, still in JSAPI. I am less certain of the gc semantics of such a beast, though.

	:Paolo Amadini
	Comment 49 • 11 years ago

Comment on attachment 809887 [details] [diff] [review]
JS Code, v5

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #48)
> Now, I suppose I could investigate reimplementing this as a method |jsval
> createWitness(string, string)|, still in JSAPI. I am less certain of the gc
> semantics of such a beast, though.

This is definitely worth investigating - maybe it will turn out it simplifies the C++ part rather than complicating it.

For the rest the patch looks good, I'm just waiting for this investigation to occur.

::: toolkit/modules/Promise.jsm
@@ +151,5 @@
> +// Import (lazily) symbol FinalizationWitness in the context
> +/**
> + * Create a new Finalization Witness.
> + *
> +* A finalization witness is an object whose sole role is to

Missing space.

::: toolkit/modules/tests/xpcshell/xpcshell.ini
@@ +14,5 @@
>  [test_sqlite.js]
>  [test_task.js]
>  [test_TelemetryTimestamps.js]
>  [test_timer.js]
> +

nit: no need to touch this file.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 50 • 11 years ago

(In reply to :Paolo Amadini from comment #49)
> Comment on attachment 809887 [details] [diff] [review]
> JS Code, v5
> 
> Review of attachment 809887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to David Rajchenbach Teller [:Yoric] from comment #48)
> > Now, I suppose I could investigate reimplementing this as a method |jsval
> > createWitness(string, string)|, still in JSAPI. I am less certain of the gc
> > semantics of such a beast, though.
> 
> This is definitely worth investigating - maybe it will turn out it
> simplifies the C++ part rather than complicating it.

Let me clarify a thing: I wrote the code using the JSAPI rather than XPCom + XPConnect for the purpose of keeping its semantics extremely simple. Involving XPConnect might make the code shorter, but it also means involving two garbage-collectors instead of one, in addition to going through an ill-documented conversion mechanism. This will considerably decrease the number of devs who actually understand that code.

I believe that this would be very much the opposite of what we're looking for in a component that relies on garbage-collection and finalization.

> For the rest the patch looks good, I'm just waiting for this investigation
> to occur.

I'll investigate, but with the reserves mentioned above.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 51 • 11 years ago

I have checked with khuey and mrbkap, who both inform me that returning a |jsval| rather than injecting the constructor should be equivalent as far as gc is concerned.

Dave, would that suit you?

	:Paolo Amadini
	Comment 52 • 11 years ago

Comment on attachment 810530 [details] [diff] [review]
Replace FinalizationWitness constructor with a xpcom/xpconnect method

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

::: toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl
@@ +24,3 @@
>     */
>    [implicit_jscontext]
> +  jsval make(jsval aTopic, jsval aString);

Can we make the arguments strings without too much trouble? They're converted to string internally in any case, and a more restrictive signature would definitely be clearer (as well as match nsIObserverService.notifyObservers):

jsval createWitness(in string aTopic, in wstring aData);

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 53 • 11 years ago

I suppose that we could, at the expense of an additional string copy.

	:Paolo Amadini
	Comment 54 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #53)
> I suppose that we could, at the expense of an additional string copy.

Good, not a big deal compared to the cost of the rest of the mechanism is.

It also makes clear which string is restricted to 8 bits and which is 16 bits.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 55 • 11 years ago

Applied paolo's feedback.

	Dave Townsend [:mossop]
	Comment 56 • 11 years ago

Comment on attachment 810569 [details] [diff] [review]
Replace FinalizationWitness constructor with a xpcom/xpconnect method, v2

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

::: toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl
@@ +25,3 @@
>     */
>    [implicit_jscontext]
> +  jsval make(in string aTopic, in wstring aString);

Yeah this looks much better to me. Thanks

	Boris Zbarsky [:bzbarsky]
	Comment 57 • 11 years ago

I recommend using ACString and AString instead of string and wstring.  For one thing, they're not lossy in the face of embedded nulls.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 58 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #57)
> I recommend using ACString and AString instead of string and wstring.  For
> one thing, they're not lossy in the face of embedded nulls.

The idea is to propagate the values to the nsIObserverService, which uses string and wstring. Does this make sense to use ACString/AString here?

	Boris Zbarsky [:bzbarsky]
	Comment 59 • 11 years ago

Ah, I guess then you lose anyway.  Doesn't matter too much where exactly you lose.  :(

	Jeff Walden [:Waldo]
	Comment 60 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #41)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40)
> > @@ +39,5 @@
> > > +                  const jschar *aValue,
> > > +                  const size_t aLenValue)
> > > +  : mTopic(aTopic, aLenTopic)
> > > +  , mValue(aValue, aLenValue)
> > > +  { }
> > 
> > This constructor indentation is still bizarre.
> 
> What did you expect?

The constructor name isn't indented properly, the member init list isn't indented properly.  You have:

class FinalizationEvent MOZ_FINAL: public nsRunnable
{
public:
FinalizationEvent(const jschar* aTopic,
                  const size_t aLenTopic,
                  const jschar *aValue,
                  const size_t aLenValue)
  : mTopic(aTopic, aLenTopic)
  , mValue(aValue, aLenValue)
  { }

I would expect to instead see:

class FinalizationEvent MOZ_FINAL: public nsRunnable
{
public:
  FinalizationEvent(const jschar* aTopic,
                    const size_t aLenTopic,
                    const jschar *aValue,
                    const size_t aLenValue)
    : mTopic(aTopic, aLenTopic)
    , mValue(aValue, aLenValue)
  { }

	Jeff Walden [:Waldo]
	Comment 61 • 11 years ago

Comment on attachment 810569 [details] [diff] [review]
Replace FinalizationWitness constructor with a xpcom/xpconnect method, v2

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

::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp
@@ +54,1 @@
>                        mValue.get());

This could all go on one line now, right?  More readable that way, seems to me.

@@ +205,5 @@
>    }
>  
> +  nsRefPtr<mozilla::FinalizationEvent> event =
> +    new mozilla::FinalizationEvent(aTopic,
> +                                   aValue);

Seems like single-lining would be good here, too.  I'd smack a |using mozilla::FinalizationEvent;| at the top of the file, too, to cut down on that wordiness.

::: toolkit/modules/Promise.jsm
@@ -183,5 @@
> -      getService(Ci.nsIFinalizationWitnessService).inject();
> -    return FinalizationWitness;
> -  },
> -  configurable: true,
> -  writeable: true,

Just in passing, "writable" was misspelled here.  :-)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 62 • 11 years ago

Comment on attachment 810569 [details] [diff] [review]
Replace FinalizationWitness constructor with a xpcom/xpconnect method, v2

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

::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp
@@ +192,5 @@
> +NS_IMETHODIMP
> +FinalizationWitnessService::Make(const char* aTopic,
> +                                 const PRUnichar* aValue,
> +                                 JSContext* cx,
> +                                 JS::Value *_retval) {

aCx, aRetval

@@ +205,5 @@
>    }
>  
> +  nsRefPtr<mozilla::FinalizationEvent> event =
> +    new mozilla::FinalizationEvent(aTopic,
> +                                   aValue);

This should be inside a namespace mozilla {} anyway

	Dave Townsend [:mossop]
	Comment 63 • 11 years ago

Comment on attachment 809770 [details] [diff] [review]
JSAPI code, v5

sr+ based on the API change in the later patch

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 65 • 11 years ago

Here's the combined version. Few bits of the code have been f+ but not r+, so asking for a review by Walden.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 66 • 11 years ago

Just to be on the safe side, asking for review, but almost nothing has changed.

	:Paolo Amadini
	Comment 67 • 11 years ago

Comment on attachment 811053 [details] [diff] [review]
JS Code, v7

Thanks a lot for doing this - this feature is extremely helpful.

Some indentation nitpicking:

XPCOMUtils.defineLazyServiceGetter(this, "FinalizationWitnessService",
                                   "@mozilla.org/toolkit/finalizationwitness;1",
                                   "nsIFinalizationWitnessService");

      let witness =
          FinalizationWitnessService.make("promise-finalization-witness", id);

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 68 • 11 years ago

Applied final feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=bc4d18f597b4

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 70 • 11 years ago

Made the test suite and the implementation more robust wrt various conditions.
I also took the opportunity to improve readability of the error messages.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 71 • 11 years ago

Adding missing installers.
Try: https://tbpl.mozilla.org/?tree=Try&rev=86a26dd93aa7

	Ryan VanderMeulen [:RyanVM]
	Comment 72 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/15dd7257c1f6
https://hg.mozilla.org/integration/fx-team/rev/6cbcb6e96dce

	Phil Ringnalda (:philor)
	Comment 73 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/15dd7257c1f6
https://hg.mozilla.org/mozilla-central/rev/6cbcb6e96dce