Open Bug 1527332 Opened 6 years ago Updated 2 years ago

Audit valuesArray code in PerformPromiseAll

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: jandem, Unassigned)

References

Details

Boris noticed this code might need an AutoRealm in the !IsWrapper case: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/js/src/builtin/Promise.cpp#2815-2838

We should investigate where/how this array is used.

Flags: needinfo?(jdemooij)

The array is exposed to user code as the value of the resolve function:

js> function C(exec) {exec(v => { dumpObject(v); }, e => {})}
js> C.resolve = v => v;
v => v
js> Promise.all.call(C, [Promise.resolve(0), Promise.resolve(1)])
({})
object 385611b00788
  global 9af3e98b060 [global]
  class 558c65c0d2b0 Array
  group 9af3e9887c0
  flags:
  proto <Array object at 9af3e9ad040>
  properties:
    "length" (shape 9af3e9b0078 permanent getterOp 558c641ddcc0 setterOp 558c641ddd20)
  elements:
      0: 0
      1: 1

Right, so a simple testcase:

  <iframe></iframe>
  <script>
  frames[0].Promise.all
           .call(Promise, [Promise.resolve(0), Promise.resolve(1)])
           .then((arg) => {
             console.log(arg instanceof Array);
             console.log(arg instanceof frames[0].Array);
           })
  </script>

This logs "true, false" in current nightly. It logs "false, true" with my changes to make the two globals share the same compartment, and logs "false, true" in Chrome and Safari. Pretty sure it should log "false, true" per spec.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

This logs "true, false" in current nightly. It logs "false, true" with my changes to make the two globals share the same compartment, and logs "false, true" in Chrome and Safari. Pretty sure it should log "false, true" per spec.

I could add a shell test based on the testcase in comment 2 (with a same-compartment global)? Then we can close this or keep it open for the pre-existing cross-compartment issue..

Flags: needinfo?(jdemooij)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(andrebargull)

Seems reasonable. Not sure how we fix the cross-compartment case, but ideally we would...

Flags: needinfo?(bzbarsky)

Adding a test-case sounds fine.

I wonder if it is possible to create the values array with the correct prototype in the cross-compartment case, because that should fix the instanceof test. I'm just not sure (because I simply don't know enough about either topic) if that'll work when one of the compartments is less-privileged resp. if it has any unintended side-effects when Xrays are involved.

Flags: needinfo?(andrebargull)
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.