Closed Bug 1297717 Opened 4 years ago Closed 4 years ago

Global objects shouldn't have a unforgeable holder

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: nox, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

The unforgeable holder is just here for performance for non-global interfaces to be initialised fast, it is unnecessary overhead to use one in the case of globals.
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Blocks: 1299306
Comment on attachment 8785021 [details] [diff] [review]
Stop using an unforgeable holder for global objects with unforgeable properties, since it's just pure slowdown in that case

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

::: dom/bindings/Codegen.py
@@ +3726,5 @@
>              $*{assertions}
>              MOZ_ASSERT(ToSupportsIsOnPrimaryInheritanceChain(aObject, aCache),
>                         "nsISupports must be on our primary inheritance chain");
>  
> +            CreateGlobal<${nativeType}, GetProtoObjectHandle>(aCx,

I was going to ask you to change the signature of CreateGlobal to return void, but it's kind of messed up already. I think it should change to bool and instead of checking !aReflector we should check the bool to return false on failure (maybe even using failureCode here). In any case, not using the return value in the only caller signals a problem with the signature :-).
Attachment #8785021 - Flags: review?(peterv) → review-
Yeah, good catch.  The code before my changes is daft too (e.g. will treat success allocating but failure to define various stuff as all ok, because aReflector is non-null)!  Will fix.
Of course if you look up the callstack, the caller kinda ignores the return value anyway, sigh.
Attachment #8785021 - Attachment is obsolete: true
Comment on attachment 8787373 [details] [diff] [review]
Stop using an unforgeable holder for global objects with unforgeable properties, since it's just pure slowdown in that case

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

Thanks for cleaning that up, it was pretty messed up already.
Attachment #8787373 - Flags: review?(peterv) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/256ea0d0de34
Stop using an unforgeable holder for global objects with unforgeable properties, since it's just pure slowdown in that case.  r=peterv
https://hg.mozilla.org/mozilla-central/rev/256ea0d0de34
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.