1033406 - The Promise object returned by Promise.jsm should still work if frozen

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[:Paolo Amadini]

The Promise object returned by Promise.jsm, like DOM Promises, should not be affected by Object.freeze calls.

Steps to reproduce from bug 1032243:

var p = new Promise(function(resolve){
   setTimeout(function(){ resolve(42); },100);
});

Object.freeze(p);

p.then(function(msg){ console.log(msg); });



Actual results:

The test never prints out `42`, presumably because the `Object.freeze(..)` is somehow affecting the way state is stored/managed for the promise instance.


Expected results:

The test should print out `42` after 100ms, and should not throw any errors.

Comment 1

[:Paolo Amadini]

The steps above work from the "about:newtab" web console.

We should add a test that freezes a Promise object in the module's test file:

./mach test toolkit/modules/tests/xpcshell/test_Promise.js

In fact, we currently "seal" the object, but we should modify its structure so that we can "freeze" it ourselves:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise-backend.js#341

Basically, we can do that by in moving all the mutable properties to an object stored in a single immutable property.

Comment 2

[Mark S. Miller]

The old title "The Promise object returned by Promise.jsm should not be affected by Object.freeze" was simply wrong, in that a frozen promise object must act differently from a non-frozen one. For example, one cannot add new own properties to a frozen promise object.

Comment 3

[:Paolo Amadini]

(In reply to Mark S. Miller from comment #2)
> The old title "The Promise object returned by Promise.jsm should not be
> affected by Object.freeze" was simply wrong, in that a frozen promise object
> must act differently from a non-frozen one. For example, one cannot add new
> own properties to a frozen promise object.

Thanks, that makes sense! Can properties be added to non-frozen Promise objects normally? In that case we could allow it in Promise.jsm promises as well, instead of freezing it ourselves as suggested in comment 1.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

Attachment: promise-freezable-test.patch

This is a test file.

Comment 5

[Tomasz Kołodziejski [:tomasz]]

Attachment: promise-freezable.patch

This is a proposed fix. I think we no longer need to further obfuscate N_STATUS and company since they are inside the obfuscated N_INTERNALS. For the same reason I don't defineProperty them. Please have a look.

Comment 6

[:Paolo Amadini]

Comment on attachment 8474698 [details] [diff] [review]
promise-freezable-test.patch

The test looks good!

+// Bug 1033406 - Make sure Promise works even after freezing.

nit: there is no need to reference the bug number, since the rest of the line already tells everything (we do reference bug numbers sometimes, when the issue is complex). "hg blame" will tell if needed.

Comment 7

[:Paolo Amadini]

Comment on attachment 8474699 [details] [diff] [review]
promise-freezable.patch

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

Very good patch, I just have some style adjustments to recommend.

::: toolkit/modules/Promise-backend.js
@@ +39,5 @@
> +const N_INTERNALS = Name("internals");
> +const N_STATUS = "status";
> +const N_VALUE = "value";
> +const N_HANDLERS = "handlers";
> +const N_WITNESS = "witness";

I agree that only N_INTERNALS should be obfuscated. The other properties can just be referenced by name, no need for the constants, for example:

this[N_INTERNALS].handlers.push(handler);

@@ +317,4 @@
>     * Internal status of the promise.  This can be equal to STATUS_PENDING,
>     * STATUS_RESOLVED, or STATUS_REJECTED.
>     */
> +  this[N_INTERNALS][N_STATUS] = STATUS_PENDING;

And here, I'd just use an object literal, indenting by two spaces:

Object.defineProperty(this, N_INTERNALS, { value: {
  /**
   * ...
   */
  value: undefined,

  /**
   * ...
   */
  handlers: [],
}});

(some comments might need re-wrapping)

For the next patch, please create a folded one incorporating the test, it makes it easier to land it in the source tree later.

Comment 8

[Tomasz Kołodziejski [:tomasz]]

Attachment: promise-freezable.patch

I totally agree that we can get rid of the N_STH constants and I did this as well as moving all the stuff into just one Object.defineProperty. Please have a look.

About the folding into one patch: in some other bug I was asked just to do the opposite so that my reviewer can see the test failing and the see the test passing. Do you know which way is more official? Separately or together? I don't have commit access so I don't care that much.

Comment 9

[Drew Willcoxon :adw]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #8)
> About the folding into one patch: in some other bug I was asked just to do
> the opposite so that my reviewer can see the test failing and the see the
> test passing. Do you know which way is more official? Separately or
> together?

There's no official way, and the answer to this type of question is usually "it depends on you and your reviewer," and they'll let you know if you can make their life easier.  Generally we keep tests and code in the same for-review patch.  The times you want to make separate patches are when your changes span multiple layers or modules of Gecko/Firefox, especially when you need to ask different people to review different changes.  For example, in bug 1009765 there two patches that touched different parts of the tree, each with a different reviewer.

Comment 10

[:Paolo Amadini]

Comment on attachment 8475279 [details] [diff] [review]
promise-freezable.patch

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

Looks good!

As Drew mentioned, there is no rule on whether the test patch should be separate, in this case it looks simple enough and I trust you've tested it for failures before applying the code fix!

At this point, a single patch (with the test nit fixed if you want) would be easier for tryserver testing and landing.

::: toolkit/modules/Promise-backend.js
@@ +34,5 @@
>  // manually when using a debugger.  They don't strictly guarantee that the
>  // properties are inaccessible by other code, but provide enough protection to
>  // avoid using them by mistake.
>  const salt = Math.floor(Math.random() * 100);
> +const N_INTERNALS = "{private:internals:" + salt + "}";

nit: The current comment might need some copyediting (it uses the plural and refers to the Name function that is now removed).

@@ +319,5 @@
> +     * status property is STATUS_REJECTED, this contains the final rejection
> +     * reason, that could be a promise, even if this is uncommon.
> +     */
> +    value: undefined,
> +  

There are spaces at end of line to remove here.

Comment 11

[Tomasz Kołodziejski [:tomasz]]

Attachment: promise-freezable.patch

I squashed two patches into one and I've addressed the issues you mentioned. It should be ready now.

Comment 12

[:Paolo Amadini]

Comment on attachment 8476043 [details] [diff] [review]
promise-freezable.patch

(In reply to Tomasz Kołodziejski [:tomasz] from comment #11)
> I squashed two patches into one and I've addressed the issues you mentioned.
> It should be ready now.

It is! Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=1602350f0c9d

Comment 13

[Tomasz Kołodziejski [:tomasz]]

As far as I remember I don't have to do anything more, right? I remember last time someone added some magic flag/whiteboard/status (or something) so that guys picking up patches will know that this patch is ready.

Thank you for all your help!

Comment 14

[:Paolo Amadini]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #13)
> As far as I remember I don't have to do anything more, right? I remember
> last time someone added some magic flag/whiteboard/status (or something) so
> that guys picking up patches will know that this patch is ready.

When the try run looks green (which does by now), you can add the checkin-needed keyword to the bug. I've just done this for you.

> Thank you for all your help!

Thanks to you for improving our Promise.jsm code :-)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3f2501008e35

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f2501008e35