Make JS::AutoValueVector subscript operator return handles

RESOLVED FIXED in mozilla32

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: Michael Shuen)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

4 years ago
Currently AutoVectorRooter and the classes derived from it have a subscript operator which returns a reference and a handleAt() method that returns a handle.  We can make subscript return handles and use that everywhere.
(Assignee)

Comment 1

4 years ago
Looking into this.
Assignee: nobody → mz_mhs-ctb
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8391971 [details] [diff] [review]
WIP 1

So far, so good. Trying to figure out how to make changes in CodeGen.py to reflect this.
(Reporter)

Comment 3

4 years ago
(In reply to Michael [:Earth4] from comment #2)
Thanks for looking at this!  Let me know if you have any questions.
(Assignee)

Comment 4

4 years ago
Sorry to bother you, but do you know how I should change up Codegen.py to use .set(), etc. instead of assignment?
Flags: needinfo?(jcoppeard)
(Reporter)

Comment 5

4 years ago
(In reply to Michael Shuen from comment #4)
That will be tricky.  My approach is usually to try building everything and see what fails, then try and work out where in the python the failing code is generated.

If you post your latest patch I will try it and see if I can give you some pointers.  Also, I'm on the #jsapi channel on IRC in European working hours so we can chat on there.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 6

4 years ago
Created attachment 8401021 [details] [diff] [review]
WIP 2

Got CodeGen working, but I'm being plagued by unexplainable build failures.
(Reporter)

Comment 7

4 years ago
Created attachment 8401374 [details] [diff] [review]
build-fixes

If you convert the last argument of MobileMessageManager::Send to be a MutableHandle, it builds fine for me.  Here's a patch to do that.
(Assignee)

Comment 8

4 years ago
Sadly, didn't work here. Keep getting syntax errors in the build system after linking libGLESv2.dll.

14:42.21 uriloader_exthandler.lib.desc
14:43.09 uriloader_prefetch.lib.desc
14:43.80 caps_src.lib.desc
14:46.30 Microsoft (R) File Expansion Utility  Version 6.2.9200.16384
14:46.30 Copyright (c) Microsoft Corporation. All rights reserved.
14:46.30
14:46.30 Adding ../../dist/bin\D3DCompiler_43.dll to Extraction Queue
14:46.32
14:46.32 Expanding Files ....
14:46.32
14:46.32 Expanding Files Complete ...
14:46.53 libGLESv2.dll
>> 14:46.58 C:/Users/Michael/AppData/Local/Temp/make13576-1.sh: line 1: syntax erro
r near unexpected token `('
14:46.58 C:/Users/Michael/AppData/Local/Temp/make13576-1.sh: line 1: `c:/mozilla
-source/mozilla-central/obj-i686-pc-mingw32/_virtualenv/Scripts/python.exe c:/mo
zilla-source/mozilla-central/config/expandlibs_exec.py --depend .deps/libGLESv2.
dll.pp --target libGLESv2.dll --uselist -- link -NOLOGO -DLL -OUT:libGLESv2.dll
-PDB:libGLESv2.pdb -SUBSYSTEM:WINDOWS -MACHINE:X86  DiagnosticsBase.obj Directiv
eHandlerBase.obj DirectiveParser.obj ExpressionParser.obj Input.obj Lexer.obj Ma

You get the idea.
(Reporter)

Comment 9

4 years ago
(In reply to Michael Shuen from comment #8)

This doesn't look related to the changes - I think this may be a problem with your build setup.  Can you try building without your patches applied to see if that works?

I've pushed a try build of the patches here which will tell us for sure: https://tbpl.mozilla.org/?tree=Try&rev=3dd7326484a6
(Assignee)

Comment 10

4 years ago
Yeah, it happens no matter what patches I build with. Do you know a way I can fix it?

Also, I guess I'll start converting handleAt users to operator [].
(Reporter)

Comment 11

4 years ago
(In reply to Michael Shuen from comment #10)

> Do you know a way I can fix it?

It looks like it might be bug 990333 - do an update and try again.
 
> Also, I guess I'll start converting handleAt users to operator [].

That would be great too!
(Assignee)

Comment 12

4 years ago
Created attachment 8408772 [details] [diff] [review]
Part 1: Convert operator[] to Handle.
Attachment #8391971 - Attachment is obsolete: true
Attachment #8401021 - Attachment is obsolete: true
Attachment #8401374 - Attachment is obsolete: true
Attachment #8408772 - Flags: review?(luke)
(Assignee)

Comment 13

4 years ago
Created attachment 8408773 [details] [diff] [review]
Part 2: Remove users of handleAt.
Attachment #8408773 - Flags: review?(luke)

Comment 14

4 years ago
I think jonco or someone from the GC team would be more qualified to review this type of patch.
Attachment #8408772 - Flags: review?(luke) → review?(terrence)
Attachment #8408773 - Flags: review?(luke) → review?(terrence)
Comment on attachment 8408772 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

Sounds like Jon has already been looking at these.
Attachment #8408772 - Flags: review?(terrence) → review?(jcoppeard)
Attachment #8408773 - Flags: review?(terrence) → review?(jcoppeard)
(Reporter)

Comment 16

4 years ago
Comment on attachment 8408772 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

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

Overall this looks good, the only thing is the use of handle.get() where handle.address() is what you really want.  Can you fix the issues below and re-upload?

::: dom/bindings/BindingUtils.h
@@ +2053,5 @@
>  inline bool
>  AddStringToIDVector(JSContext* cx, JS::AutoIdVector& vector, const char* name)
>  {
>    return vector.growBy(1) &&
> +         InternJSString(cx, (jsid&)(vector[vector.length() - 1].get()), name);

Please change this to use *handle.address(), rather than using handle.get() and a c-style cast to get rid of the const.

::: js/src/ctypes/CTypes.cpp
@@ +5135,5 @@
>  
>    for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) {
>      const FieldInfoHash::Entry& entry = r.front();
>      // Add the field descriptor to the array.
> +    if (!AddFieldToArray(cx, (jsval*)(&fieldsVec[entry.value().mIndex].get()),

Please use handle.address() here rather than &handle.get().

@@ +5946,5 @@
>        if (!vec.resize(len))
>          return false;
>  
>        for (size_t i = 0; i < len; ++i)
> +        vec[i].set(JS::ObjectValue(*fninfo->mArgTypes[i]));

You can do handle.setObject(*objPtr) so you don't need the JS::ObjectValue() here.

@@ +6182,5 @@
>    for (uint32_t i = 0; i < cif->nargs; ++i) {
>      // Convert each argument, and have any CData objects created depend on
>      // the existing buffers.
>      RootedObject argType(cx, fninfo->mArgTypes[i]);
> +    if (!ConvertToJS(cx, argType, NullPtr(), args[i], false, false, (jsval*)(&argv[i].get())))

Please use address() here.

::: js/src/jsscript.cpp
@@ +234,5 @@
>          for (uint32_t i = 0; i < nameCount; i++) {
>              RootedAtom atom(cx);
>              if (!XDRAtom(xdr, &atom))
>                  return false;
> +            atoms[i].set(StringValue(atom));

You can use handle.setString(atom) here.

::: js/src/jswrapper.cpp
@@ +410,5 @@
>              RootedValue v(cx, StringValue(ni->begin()[i]));
>              if (!ValueToId<CanGC>(cx, v, &id))
>                  return false;
>              keys.infallibleAppend(id);
> +            if (!origin->wrapId(cx, (jsid*)(&keys[i].get())))

Please use handle.address() here.

::: js/src/vm/Debugger.cpp
@@ +5168,5 @@
>          for (size_t i = 0; i < n; i++) {
>              if (!rewrappedIds.append(JSID_VOID) || !rewrappedDescs.append())
>                  return false;
>              id = ids[i];
> +            if (!unwrappedDescs[i].wrapInto(cx, obj, id, (jsid*)(&rewrappedIds[i].get()), &rewrappedDescs[i]))

Ditto here.
Attachment #8408772 - Flags: review?(jcoppeard)
(Reporter)

Comment 17

4 years ago
Comment on attachment 8408773 [details] [diff] [review]
Part 2: Remove users of handleAt.

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

Looks good!
Attachment #8408773 - Flags: review?(jcoppeard) → review+
(Reporter)

Comment 18

4 years ago
(In reply to Jon Coppeard (:jonco) from comment #17)
I found there's another use of handleAt in ToJSValue() in dom/bindings/ToJSValue.h - please convert this one too.

Cheers!
(Assignee)

Comment 19

4 years ago
Created attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.
Attachment #8408772 - Attachment is obsolete: true
Attachment #8411434 - Flags: review?(jcoppeard)
(Assignee)

Comment 20

4 years ago
Comment on attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

Oops. I guess I missed some.
Attachment #8411434 - Flags: review?(jcoppeard)
(Assignee)

Comment 21

4 years ago
Created attachment 8411437 [details] [diff] [review]
Part 2: Remove users of handleAt.
Attachment #8408773 - Attachment is obsolete: true
Attachment #8411437 - Flags: review?(jcoppeard)
(Assignee)

Comment 22

4 years ago
Comment on attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

Well, it seems to be fine. I was a little worried about .get() usage in a few spots, but it seems to be OK.
Attachment #8411434 - Flags: review?(jcoppeard)
(Reporter)

Comment 23

4 years ago
Comment on attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

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

This all looks good.

I saw that there were some changes to CTypes.cpp in the second patch that should be part of this patch.  That's fine but please combine the two patches into one before pushing, otherwise people trying to bisect will trip over the broken revision.

Next we need to get the browser parts reviewed.

::: dom/bindings/Codegen.py
@@ +12030,5 @@
>                  " else if (argc == %d) {\n"
>                  "  // This is our current trailing argument; reduce argc\n"
>                  "  --argc;\n"
>                  "} else {\n"
> +                "  argv[%d].set(JS::UndefinedValue());\n"

You can use setUndefined() for this.

::: js/src/ctypes/CTypes.cpp
@@ +4827,5 @@
>        RootedObject fieldType(cx, nullptr);
>        Rooted<JSFlatString*> name(cx, ExtractStructField(cx, item, fieldType.address()));
>        if (!name)
>          return false;
> +      fieldRoots[i].set(JS::ObjectValue(*fieldType));

You can use setObject() here.
Attachment #8411434 - Flags: review?(jcoppeard) → review+
(Reporter)

Updated

4 years ago
Attachment #8411437 - Flags: review?(jcoppeard) → review+
(Reporter)

Updated

4 years ago
Attachment #8411434 - Flags: review?(bugs)
(Reporter)

Updated

4 years ago
Attachment #8411437 - Flags: review?(bugs)
Comment on attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

This patch seems to have some random tab characters. Should use only
spaces for indentation.
Attachment #8411434 - Flags: review?(bugs) → review-
Attachment #8411437 - Flags: review?(bugs) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 8412191 [details] [diff] [review]
Patch (v3)
Attachment #8411434 - Attachment is obsolete: true
Attachment #8411437 - Attachment is obsolete: true
Attachment #8412191 - Flags: review?(jcoppeard)
Attachment #8412191 - Flags: review?(bugs)
(Assignee)

Comment 26

4 years ago
Created attachment 8412193 [details] [diff] [review]
subscript-operator.patch

Pulling tip broke the patch apply. Fixed that.
Attachment #8412191 - Attachment is obsolete: true
Attachment #8412191 - Flags: review?(jcoppeard)
Attachment #8412191 - Flags: review?(bugs)
Attachment #8412193 - Flags: review?(jcoppeard)
Attachment #8412193 - Flags: review?(bugs)
So how is attachment 8412193 [details] [diff] [review] different from the patch I gave r+?
Attachment #8412193 - Flags: review?(bugs) → review+
(Assignee)

Comment 28

4 years ago
I merged the two together (to avoid problems when bisecting) and you gave r- on the other one.
(Reporter)

Comment 29

4 years ago
Comment on attachment 8412193 [details] [diff] [review]
subscript-operator.patch

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

Great!
Attachment #8412193 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 30

4 years ago
Created attachment 8412915 [details] [diff] [review]
Patch for checkin
Attachment #8412193 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
This is bitrotted. Please rebase.
Keywords: checkin-needed
(Assignee)

Comment 32

4 years ago
Created attachment 8414105 [details] [diff] [review]
Unbitrotted
Attachment #8412915 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed9e9e160f2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ed9e9e160f2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.