Closed Bug 995163 Opened 6 years ago Closed 6 years ago

keep the global with the value in Promise in NativePromiseCallback

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch native.patch (obsolete) — Splinter Review
No description provided.
Attachment #8405324 - Flags: feedback?(bzbarsky)
Thanks Andrea!
Blocks: 949325
Comment on attachment 8405324 [details] [diff] [review]
native.patch

We discussed this on IRC, but this code is assuming that AppendNativeHandler is called with JS on the stack, which seems like a pretty bold assumption...
Attachment #8405324 - Flags: feedback?(bzbarsky) → feedback-
Attached patch wrapper1.patch (obsolete) — Splinter Review
After the discussion by email, is this a reasonable approach to fix this issue?
Attachment #8405324 - Attachment is obsolete: true
Attachment #8407447 - Flags: review?(bzbarsky)
Comment on attachment 8407447 [details] [diff] [review]
wrapper1.patch

>+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mWrapper)

You don't need to add that member.  Please don't.

In RunTask, you can just use GetOrCreateWrapper.

Also, don't you need to eneter the compartment of the wrapper and wrap the value into it or something?  If performace is a concern, you can use MaybeWrapValue().

So what I would propose is that in RunTask you remove the "just for rooting" comment, call GetOrCreateWrapper, enter its compartment (or bail out if null is returned: that means OOM), wrap the value into the context compartment, and then pass cx to the callbacks along with the value.
Attachment #8407447 - Flags: review?(bzbarsky) → review-
Attached patch wrapper1.patchSplinter Review
Attachment #8407447 - Attachment is obsolete: true
Attachment #8414337 - Flags: review?(bzbarsky)
Comment on attachment 8414337 [details] [diff] [review]
wrapper1.patch

r=me
Attachment #8414337 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/5b722269feee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.