Remove js::ReportASCIIErrorWithId

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: arai, Assigned: pass2pawan, Mentored)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

9 months ago
separated from bug 1289050.

js::ReportASCIIErrorWithId is used only in one file.
the function should be removed.
(Reporter)

Comment 1

9 months ago
no, not blocking :P
No longer blocks: 1289050
See Also: → bug 1289050
(Assignee)

Comment 2

7 months ago
i'd like to take this bug , is it a good first bug ?
(Reporter)

Comment 3

7 months ago
yes, I'm happy to mentor.
I'll post the details shortly.
If you have any question in advance, feel free to ask in comment.
Mentor: arai.unmht@gmail.com
(Assignee)

Comment 4

7 months ago
This will be my first bug , although i'll fix the bug , please do help me through the process of patching and sending it for review .
(Reporter)

Comment 5

7 months ago
js::ReportASCIIErrorWithId is declared in js/src/jsfriendapi.h

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/src/jsfriendapi.h#2773-2774
> JS_FRIEND_API(void)
> ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);


and defined in js/src/jsfriendapi.cpp

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/src/jsfriendapi.cpp#1344-1357
> JS_FRIEND_API(void)
> js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)
> {
>     RootedValue idv(cx);
>     if (!JS_IdToValue(cx, id, &idv))
>         return;
>     RootedString idstr(cx, JS::ToString(cx, idv));
>     if (!idstr)
>         return;
>     JSAutoByteString bytes;
>     if (!bytes.encodeUtf8(cx, idstr))
>         return;
>     JS_ReportErrorUTF8(cx, msg, bytes.ptr());
> }


but it's used only from 1 file, js/xpconnect/wrappers/AddonWrapper.cpp

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/xpconnect/wrappers/AddonWrapper.cpp#234
> template<typename Base>
> bool
> AddonWrapper<Base>::defineProperty(JSContext* cx, HandleObject wrapper, HandleId id,
>                                    Handle<PropertyDescriptor> desc,
>                                    ObjectOpResult& result) const
> {
> ...
>     js::ReportASCIIErrorWithId(cx, "unable to modify interposed property %s", id);
>     return false;
> }

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/xpconnect/wrappers/AddonWrapper.cpp#250
> template<typename Base>
> bool
> AddonWrapper<Base>::delete_(JSContext* cx, HandleObject wrapper, HandleId id,
>                             ObjectOpResult& result) const
> {
> ...
>     js::ReportASCIIErrorWithId(cx, "unable to delete interposed property %s", id);
>     return false;
> }

So, it's not necessarily be in jsfriendapi, but it should be the file static function.

Required code change:
  * Move js::ReportASCIIErrorWithId to js/xpconnect/wrappers/AddonWrapper.cpp and change it to file static function
  * Remove declaration of ReportASCIIErrorWithId from js/src/jsfriendapi.h

I assume you've already built firefox once, so, you need to apply the change, and then re-build to see it works.

About process, the following documentation will help:
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

so, basically, push your changeset to review repository, or export it as a patch file and attach here
(Assignee)

Comment 6

7 months ago
Created attachment 8817367 [details] [diff] [review]
bug-1306538.patch
(Assignee)

Comment 7

7 months ago
It does work after i rebuilt it , and ive exported it as patch here , what do i do next
(Reporter)

Comment 8

7 months ago
Comment on attachment 8817367 [details] [diff] [review]
bug-1306538.patch

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

Thank you, but unfortunately this is not correct changeset to export.
please commit your change before exporting.
  hg commit -m 'Bug 1306538 - Move js::ReportASCIIErrorWithId to AddonWrapper.cpp.'
  hg export -r . > ../bug-1306538.patch
(Assignee)

Comment 9

7 months ago
Created attachment 8817394 [details] [diff] [review]
bug-1306538.patch
Attachment #8817367 - Attachment is obsolete: true
(Assignee)

Comment 10

7 months ago
Arai is that okay now ?
(Reporter)

Comment 11

7 months ago
no
the patch file is a text file that contains the diff of your change, so you can check if it contains correct change, by opening it with text editor.

then, it still contains the same thing as you posted first, so I think hg commit failed for some reason.
can you check the output of hg commit?
also, what does "hg status" say?
(Assignee)

Comment 12

7 months ago
hg commit gives me the output "created new head" . 
hg status gives me "? bug-1306538.patch".
(Assignee)

Comment 13

7 months ago
and i tried to do this , is this right https://pastebin.mozilla.org/8950685
(Reporter)

Comment 14

7 months ago
yes, it's now exporting your changeset.
now, apply proper change and try again
(Assignee)

Comment 15

7 months ago
What do you mean by proper change ? 
and do i run hg commit again and then export it to a patch ?
(Reporter)

Comment 16

7 months ago
Do you see the content of the patch you posted?
it doesn't contain the change mentioned in comment #5.

If you've already applied some other changes than the patch above, maybe it's in other changeset than the one in above pastebin.
If so, can you check the output of "hg log -f -l10" ?
it will print the information of topmost 10 changesets
If you see your name twice, it will mean that you've committed twice, and the expected change may be in other one,
in that case, you may need to fold them into one, so, please post the output of hg log here, or in IRC and ask the way to fold them properly.

if you haven't changed, please apply the change and then do "hg commit --amend -m ..." to update the topmost changeset, and then export it again.
(Assignee)

Comment 17

7 months ago
https://pastebin.mozilla.org/8950706 
My name was there twice as you sad there would be .
(Assignee)

Comment 18

7 months ago
Arai i have folded the changesets can you tell me the commands i need to run after this?
(Assignee)

Comment 19

7 months ago
Created attachment 8817775 [details] [diff] [review]
bug-1306538.patch
Attachment #8817394 - Attachment is obsolete: true
(Reporter)

Comment 20

7 months ago
looks like you've folded wrong changesets and also exported patch file, as you see totally unrelated files in the patch (next time, try opening the patch file in text editor and check if it contains correct changes)
it would be better remove the changeset and create from clean tree again
you can remove the topmost changeset by
  hg strip -r .
(Assignee)

Comment 21

7 months ago
Created attachment 8817783 [details] [diff] [review]
bug-1306538.patch
Attachment #8817775 - Attachment is obsolete: true
(Assignee)

Comment 22

7 months ago
This should finally do it , thanks a lot for all your help .
(Reporter)

Comment 23

7 months ago
Comment on attachment 8817783 [details] [diff] [review]
bug-1306538.patch

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

Thank you for update :)

It's almost there.  Can you update the patch to address the following comments, and post it again and ask review?
(you can ask review in attachment form, by choosing "?" in "review" field, and fill the text area next to it with email of me)

::: js/src/jsfriendapi.h
@@ +2785,1 @@
>  ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);

please remove all those 3 lines.
it's no more exported from jsfriendapi

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +18,5 @@
>  
>  #include "nsID.h"
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/

This comment is not necessary.
please remove them.

@@ +19,5 @@
>  #include "nsID.h"
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)

JS_FRIEND_API macro is used only in jsfriendapi
so, it's not necessary here.
please change it to just "static inline void"

also, this definition should be inside namespace xpc below,
also after "using namespace js" and "using namespace JS".

@@ +20,5 @@
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)

This shouldn't be in js namespace.
please remove "js::"

@@ +22,5 @@
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)
> +{
> +	RootedValue idv(cx);

Please use 4 spaces indent, instead of tab.
Attachment #8817783 - Flags: feedback+
(Assignee)

Comment 24

6 months ago
Created attachment 8817955 [details] [diff] [review]
bug-1306538.patch
Attachment #8817783 - Attachment is obsolete: true
Attachment #8817955 - Flags: review?(arai.unmht)
(Reporter)

Comment 25

6 months ago
Comment on attachment 8817955 [details] [diff] [review]
bug-1306538.patch

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

Thanks again.
can you update the patch to follow the review comments?

::: js/src/jsfriendapi.cpp
@@ +1339,5 @@
>  {
>      return ReportIsNotFunction(cx, v, -1);
>  }
>  
> +

please remove these 2 empty lines

::: js/src/jsfriendapi.h
@@ +2785,1 @@
>  ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);

Please remove this declaration.
we don't need prototype here.
since this is now file static function in AddonWrapper.cpp.

(also please remove empty line above, so that there's only 1 empty line between 2 declarations)

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
> +static inline void
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg,HandleId id)

1. move this under "namespace xpc {" line
2. remove "js::" prefix, since this should be file static.
3. add space after "msg," that was there before you move this line
Attachment #8817955 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 26

6 months ago
when i move it under the xpc namespace and remove js:: prefix i get a compilation error , because the compiler cannot distinguish between xpc:: and js:: namespace .
(Reporter)

Comment 27

6 months ago
can you post the current patch and the error you get?
(Assignee)

Comment 28

6 months ago
Created attachment 8818200 [details] [diff] [review]
bug-1306538.patch
Attachment #8817955 - Attachment is obsolete: true
Attachment #8818200 - Flags: review?(arai.unmht)
(Reporter)

Comment 29

6 months ago
can you also post the compiler output ?
(Assignee)

Comment 30

6 months ago
it worked this time , i was getting an error before because there was a function declaration in jsfriendapi.h , so the final compiler output was build successful
(Reporter)

Comment 31

6 months ago
Comment on attachment 8818200 [details] [diff] [review]
bug-1306538.patch

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

Okay, it's almost there.
only cosmetic changes.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +

please remove this empty line

@@ +25,2 @@
>  namespace xpc {
> +	

please remove the trailing white spaces.
Attachment #8818200 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 32

6 months ago
Created attachment 8818215 [details] [diff] [review]
bug-1306538.patch
Attachment #8818200 - Attachment is obsolete: true
Attachment #8818215 - Flags: review?(arai.unmht)
(Assignee)

Comment 33

6 months ago
Thanks a lot for all your help :)
(Reporter)

Comment 34

6 months ago
Comment on attachment 8818215 [details] [diff] [review]
bug-1306538.patch

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

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +namespace xpc {	

there's still trailing whitespaces (actually a tab).
it would be nice to configure your text editor to highlight the trailing whitespaces, so that you can notice it before creating a patch.

also, please insert one empty line before "static inline void" below.
Attachment #8818215 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 35

6 months ago
Created attachment 8818216 [details] [diff] [review]
bug-1306538.patch
Attachment #8818215 - Attachment is obsolete: true
Attachment #8818216 - Flags: review?(arai.unmht)
(Assignee)

Comment 36

6 months ago
i havent actually been able to see what you mean by whitespace ?
(Reporter)

Comment 37

6 months ago
Comment on attachment 8818216 [details] [diff] [review]
bug-1306538.patch

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

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +namespace xpc {	

uh, it's still there :P
you can check the highlighted tab in review page
(click [review] link of the patch, and click "AddonWrapper.cpp" filename)
Attachment #8818216 - Flags: review?(arai.unmht) → feedback+
(Reporter)

Comment 38

6 months ago
(In reply to pass2pawan from comment #36)
> i havent actually been able to see what you mean by whitespace ?

you've added a tab (HTAB) after "namespace xpc {" line.
the change should be reverted.
(Assignee)

Comment 39

6 months ago
Created attachment 8818220 [details] [diff] [review]
bug-1306538.patch

Finally :P
Attachment #8818216 - Attachment is obsolete: true
Attachment #8818220 - Flags: review?(arai.unmht)
(Reporter)

Comment 40

6 months ago
Comment on attachment 8818220 [details] [diff] [review]
bug-1306538.patch

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

Great :D
Thank you again for your patch!

Okay, here's remaining things to do:
  1. push this patch to try server (I'll do this)
  2. check the try server result (after 2-3 hours)
  3. add "checkin-needed" keyword if the try is green (means no failure)
  4. wait for the patch to be landed to mozilla-inbound repository
  5. wait for the patch to be merged to mozilla-central
Attachment #8818220 - Flags: review?(arai.unmht) → review+
(Reporter)

Comment 41

6 months ago
here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9e88962070a6461d41962bdf04a79620e396838

when it shows 100% and there's no failure (or only known intermittent failure), you can add "checkin-needed" keyword
(Assignee)

Comment 42

6 months ago
is it the same as in checking the box of checkin-needed like i did with review ?
(Reporter)

Comment 43

6 months ago
In "Keywords" field at the top of bug page, you can add it.
(Assignee)

Comment 44

6 months ago
ok thank you and will i be assigned this bug at the completion of the entire procedure ?
(Reporter)

Comment 45

6 months ago
oops, sorry I forgot to assign it to you.
basically you'll be assigned when you post a patch.
Assignee: nobody → pass2pawan
Status: NEW → ASSIGNED
(Assignee)

Comment 46

6 months ago
arai , i dont know how to evaluate the try tests  there are 7 unclassified errors , 6 of them intermittent one of them said the build failed but when i saw the log all of them were a success . Can i put in checkin-needed or is there something wrong ?
(Reporter)

Comment 47

6 months ago
(In reply to pass2pawan from comment #46)
> arai , i dont know how to evaluate the try tests  there are 7 unclassified
> errors , 6 of them intermittent one of them said the build failed but when i
> saw the log all of them were a success . Can i put in checkin-needed or is
> there something wrong ?

Yes, they're all intermittent.
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 48

6 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3561e1497acf
Move js::ReportASCIIErrorWithId to AddonWrapper.cpp. r=arai
Keywords: checkin-needed

Comment 49

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3561e1497acf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No need to backport this to aurora
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.