1474272 - Stop using js::GetGlobalForObjectCrossCompartment in xpc::NativeGlobal

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

This is sort of a big one with 51 callers, although most of them are easy to audit.

Also, NativeGlobal currently always gets the object's global even though most callers already pass in a global. It would be nice to make NativeGlobal assert JS_IsGlobalObject and then have a separate function for the non-global object case.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove unused WrappedJSToDictionary

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use more precise types in Promise code and add some assertions

Just some minor clean up that makes it easier to reason about some of the NativeGlobal calls.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994801 [details] [diff] [review]
Part 1 - Remove unused WrappedJSToDictionary

Looks like this was used by some FxOS code at one point...  r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994814 [details] [diff] [review]
Part 2 - Use more precise types in Promise code and add some assertions

r=me

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Call AssertHeapIsIdleOrIterating instead of AssertHeapIsIdle in JS::GetNonCCWObjectGlobal

With the later patches we can call this function in the memory reporting code while iterating the heap. We're replacing some js::GetGlobalForObjectCrossCompartment calls and that one doesn't assert anything about the heap state.

Comment 6

[Jan de Mooij [:jandem]]

I could also remove the assertion if you think that's fine..

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Make NativeGlobal only work with global objects and add NonCCWObjectNativeGlobal for other callers

xpc::NativeGlobal is now faster because it doesn't have to look up the object's global. xpc::NonCCWObjectNativeGlobal is nice to be explicit about not having a CCW object.

This is green on top of the patches in the dependent bugs.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8995149 [details] [diff] [review]
Part 3 - Call AssertHeapIsIdleOrIterating instead of AssertHeapIsIdle in JS::GetNonCCWObjectGlobal

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

This is fine like this.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Make NativeGlobal only work with global objects and add NonCCWObjectNativeGlobal for other callers

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8995155 [details] [diff] [review]
Part 4 - Make NativeGlobal only work with global objects and add NonCCWObjectNativeGlobal for other callers

I'd like bholley to take a look.  I will admit to not being a fan of the length of NonCCWObjectNativeGlobal.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Stop using js::GetGlobalForObjectCrossCompartment in xpc::NativeGlobal

After our discussion yesterday, I think Bobby will like this one more.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6402132e9f
part 1 - Remove unused WrappedJSToDictionary. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9247183eb7
part 2 - Use more precise types in Promise code and add some assertions. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e8a6239f8e
part 3 - Call AssertHeapIsIdleOrIterating instead of AssertHeapIsIdle in JS::GetNonCCWObjectGlobal. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/f486f734d4c4
part 4 - Stop using js::GetGlobalForObjectCrossCompartment in xpc::NativeGlobal. r=bholley

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6d6402132e9f
https://hg.mozilla.org/mozilla-central/rev/8d9247183eb7
https://hg.mozilla.org/mozilla-central/rev/53e8a6239f8e
https://hg.mozilla.org/mozilla-central/rev/f486f734d4c4