Closed Bug 862115 Opened 7 years ago Closed 6 years ago

Use Rooted<JSPropertyDescriptor> in favor of JSPropertyDescriptor::AutoRooter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: terrence, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

This should be straightforward and allow us to remove the AutoRooter variant.
Blocks: ExactRooting
No longer blocks: GenerationalGC
Assignee: general → terrence
Attached patch v0 (obsolete) — Splinter Review
Attachment #743773 - Flags: review?(jcoppeard)
Attachment #743773 - Flags: feedback?(jwalden+bmo)
Comment on attachment 743773 [details] [diff] [review]
v0

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

Looks good!

::: js/src/jsapi.h
@@ +3358,5 @@
>      bool hasAttributes(unsigned attrs) const { return desc()->attrs & attrs; }
>  
>      JS::MutableHandleObject object() { return JS::MutableHandleObject::fromMarkedLocation(&desc()->obj); }
>      unsigned attributes() const { return desc()->attrs; }
> +    unsigned &attributesRef() { return desc()->attrs; }

Should this just be attributes() like the versions of getter() and setter() that return a reference below?
Attachment #743773 - Flags: review?(jcoppeard) → review+
Comment on attachment 743773 [details] [diff] [review]
v0

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

Nothing too goofy sticks out here.  Although I was a bit saddened to see that PropertyDescriptor has isPermanent(), isReadonly(), etc. as the field names, rather than adopting the standard-sanctioned inverted names (isConfigurable(), isWritable()).  But I guess that's pre-existing, so meh.

Oh, and exposing refs to the internal fields feels a bit weird.  Especially attributesRef -- you could just as easily fill in a local and then do a setAttributes() after that, which seems easier to reason about/understand, to me.
Attachment #743773 - Flags: feedback?(jwalden+bmo) → feedback+
Blocks: 795030
No longer blocks: ExactRooting
Attached patch WIP: xpconnect (obsolete) — Splinter Review
Attached patch WIP: browser (obsolete) — Splinter Review
Blocks: 898608
No longer blocks: 795030
Assignee: terrence → jcoppeard
Attachment #743773 - Attachment is obsolete: true
Attachment #787478 - Flags: review?(terrence)
Attachment #786357 - Attachment is obsolete: true
Attachment #787479 - Flags: review?(bobbyholley+bmo)
Attachment #786359 - Attachment is obsolete: true
Attachment #787480 - Flags: review?(bugs)
Attachment #787482 - Flags: review?(bugs)
Comment on attachment 787479 [details] [diff] [review]
propdesc-patch-xpc

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

Nice. r=bholley
Attachment #787479 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 787480 [details] [diff] [review]
propdesc-patch-browser

>   if (!proto) {
>-    desc->obj = NULL;
>+    desc.object().set(NULL);
s/NULL/nullptr/

>-FillPropertyDescriptor(JSPropertyDescriptor* desc, JSObject* obj, bool readonly)
>+FillPropertyDescriptor(JS::MutableHandle<JSPropertyDescriptor> desc, JSObject* obj, bool readonly)
> {
>-  desc->obj = obj;
>-  desc->attrs = (readonly ? JSPROP_READONLY : 0) | JSPROP_ENUMERATE;
>-  desc->getter = NULL;
>-  desc->setter = NULL;
>-  desc->shortid = 0;
>+  desc.object().set(obj);
>+  desc.setAttributes((readonly ? JSPROP_READONLY : 0) | JSPROP_ENUMERATE);
>+  desc.setGetter(NULL);
>+  desc.setSetter(NULL);
>+  desc.setShortId(0);
while you're here, nullptr please
Attachment #787480 - Flags: review?(bugs) → review+
Comment on attachment 787482 [details] [diff] [review]
propdesc-patch-ipc


>     if (!in.setter()) {
>-        out->setter = NULL;
>+        out.setSetter(NULL);
Want to change NULL to nullptr while you're here.
Attachment #787482 - Flags: review?(bugs) → review+
Comment on attachment 787478 [details] [diff] [review]
propdesc-patch-js

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

Great! r=me

::: js/src/jsproxy.cpp
@@ +126,5 @@
>          vp.setUndefined();
>          return true;
>      }
> +    if (!desc.getter() ||
> +        (!desc.hasGetterObject() && desc.getter() == JS_PropertyStub)) {

{ on new line, while you're here.
Attachment #787478 - Flags: review?(terrence) → review+
Comment on attachment 787479 [details] [diff] [review]
propdesc-patch-xpc

>     nsGlobalWindow *win;
>-    if (!desc->obj && Traits::Type == XrayForWrappedNative && JSID_IS_STRING(id) &&
>+    if (!desc.object() && Traits::Type == XrayForWrappedNative && JSID_IS_STRING(id) &&
>         (win = static_cast<nsGlobalWindow*>(As<nsPIDOMWindow>(wrapper))))
>     {
>         nsDependentJSString name(id);
>         nsCOMPtr<nsIDOMWindow> childDOMWin = win->GetChildWindow(name);

For some reason, this s/->obj/.object()/ replacement makes GCC 4.8 complain about 'win' being maybe unitialized:

js/xpconnect/wrappers/XrayWrapper.cpp:1419:70: error: 'win' may be used uninitialized in this function [-Werror=maybe-uninitialized]
          nsCOMPtr<nsIDOMWindow> childDOMWin = win->GetChildWindow(name);
                                                                       ^
That's a build warning, but it breaks the build for people building w/ 4.8 and --enable-warnings-as-errors, because this directory is FAIL_ON_WARNINGS.

I don't see how that change could make 'win' maybe-uninitialized, but maybe I'm missing something... Maybe it's a compiler bug?
(After a bit more digging, I think we should probably just prevent the warning in comment 15 from being tracked by FAIL_ON_WARNINGS, since it's false-positive-ish. That'll prevent it from causing build bustage. I filed bug 903513 on that.)
(In reply to Daniel Holbert [:dholbert] from comment #16)

Great, thanks for sorting this out!
https://hg.mozilla.org/mozilla-central/rev/db34065a8666
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.