Closed Bug 1152902 Opened 5 years ago Closed 5 years ago

Add a fast path to promises for being resolved with a promise

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 3 obsolete files)

Instead of taking the generic thenable path, I think we should be able to do better.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
There's actually an API question here, though.  I can expose the function as I did, or we can add a boolean-returning thing like PromiseBinding::IsThenMethod() which takes a JSObject* and does the checks under the hood.  Then the caller won't need to be aware of the promiseWrapper complexities...  Peter, do you have a preference between those?
Flags: needinfo?(peterv)
Comment on attachment 8590401 [details] [diff] [review]
part 2.  Add a fast path for the case when a Promise is resolved with another Promise

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

::: dom/promise/Promise.cpp
@@ +1190,5 @@
> +          NS_SUCCEEDED(UNWRAP_OBJECT(Promise, valueObj, nextPromise))) {
> +        // If we were taking the codepath that involves ThenableResolverTask and
> +        // PromiseInit below, then eventually, in ThenableResolverTask::Run, we
> +        // would create some JSFunctions in the compartment of
> +        // this->GetWrapper() an pass them to the PromiseInit. So by the time

Nit: s/an/and
Attachment #8590401 - Flags: review?(nsm.nikhil) → review+
Talked to Peter.  We're going to set things up so we have a mozilla::dom::PromiseBinding::IsThen() method and a [MethodIdentityTestable] extended attr.
Attachment #8590400 - Attachment is obsolete: true
Attachment #8590400 - Flags: review?(peterv)
Attachment #8590401 - Attachment is obsolete: true
Flags: needinfo?(peterv)
Attachment #8592272 - Attachment is obsolete: true
Attachment #8592272 - Flags: review?(peterv)
Attachment #8592276 - Flags: review?(peterv) → review+
Going to have to back this out once IT's tree-closing window ends, the tomfoolery in https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/test/unit/settings/layout_item_test.js doesn't seem to like it, https://treeherder.mozilla.org/logviewer.html#?job_id=9021345&repo=mozilla-inbound (which is Gu, gaia-unit, in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c503b0418516, thanks for the utterly useless logs TaskCluster).
OK.  Dealing with trying to debug what gaia-unit-test is doing is just not worth it to me, given how painful it is to run locally.  If someone wants to take over this bug, please feel free to.
Assignee: bzbarsky → nobody
Status: ASSIGNED → NEW
Actually, I think I know what's going on here.  The new code changed the ordering of promise callbacks in a way we apparently had no test for.  Fixing that makes Gu green on try.
Assignee: nobody → bzbarsky
Comment on attachment 8594424 [details] [diff] [review]
Patch on top of part 2 to fix the ordering issue

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

Sorry, I should have caught the lack of async-ness in the first patch.
Attachment #8594424 - Flags: review?(nsm.nikhil) → review+
https://hg.mozilla.org/mozilla-central/rev/90f39251be10
https://hg.mozilla.org/mozilla-central/rev/e372fae4d9b6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.