1119682 - JavaScript strict warning: nsSearchSuggestions.js, line 34: TypeError: setting a property that has only a getter

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Jeff Walden [:Waldo]]

SuggestAutoComplete.prototype = {
  ...
  get _suggestionLabel() {
    delete this._suggestionLabel;
    let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
    return this._suggestionLabel = bundle.GetStringFromName("suggestion_label");
  },

Line 34 is the return/assignment.  Given this is in a prototype, it's almost certainly the case that |delete this._suggestionLabel| fails to to delete off an SuggestAutoComplete instance (the property's on the prototype, and deletion only affects own properties), then fails to assign because the assignment target is still a getter with no setter.

Should be an easy bug to fix -- remove the deletion, add a |let label = ...| for the label string, use Object.defineProperty(SuggestAutoComplete.prototype, "_suggestionLabel", { value: label }), then return label.

Comment 1

[Prathik Sai [:Skandan]]

I would love to take this on, but it might take me a few days ! I have to get back to college tomorrow where I have a few registration procedures, would that be fine?

Comment 2

[:Gijs (he/him)]

(In reply to Prathik Sai  [:Skandan] from comment #1)
> I would love to take this on, but it might take me a few days ! I have to
> get back to college tomorrow where I have a few registration procedures,
> would that be fine?

Sure, no rush.

Comment 3

[Prathik Sai [:Skandan]]

Thank You Gijs! :-) I'm still a beginner at JS, but I'll work A.S.A.P.

Comment 4

[Jeff Walden [:Waldo]]

Since I neglected to include it in comment 0: clicking on the magnifying-glass search icon in a browser with a fresh profile, with the search dropdown not already open (can't remember if the search box contained any text or not), was enough to consistently trigger the warning for me, and should be an easy way to verify the warning is gone.

Comment 5

[Prathik Sai [:Skandan]]

Attachment: Bug-1119682.diff (suggested solution implemented)

Can you tell me how I can reproduce the error? I tried using Ctrl+Shift+J and then  clicked the magnifying glass, but I seem to be doing something wrong. And please do tell me if I've done something wrong!

Comment 6

[:Gijs (he/him)]

Comment on attachment 8547199 [details] [diff] [review]
Bug-1119682.diff (suggested solution implemented)

(In reply to Prathik Sai  [:Skandan] from comment #5)
> Created attachment 8547199 [details] [diff] [review]
> Bug-1119682.diff (suggested solution implemented)
> 
>  Can you tell me how I can reproduce the error? I tried using Ctrl+Shift+J
> and then  clicked the magnifying glass, but I seem to be doing something
> wrong. And please do tell me if I've done something wrong!

Try setting "javascript.options.strict" to true in about:config. That should help in reproducing this.

Off-hand, the patch seems fine, save for some whitespace nits (please put spaces after the commas between arguments to a function call, and no space after the opening { of an object).

Comment 7

[Prathik Sai [:Skandan]]

Attachment: Bug-1119682.diff (white space nits fixed)

:) I'm sorry about the white space nits! Was concentrating more on the JS I forgot to hold to the general conventions. Working on keeping to standards.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8547218 [details] [diff] [review]
Bug-1119682.diff (white space nits fixed)

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

This looks good to me. Please adopt the nit below and then request review from ":MattN" or ":felipe" - sadly, I'm not a reviewer for the toolkit module which this changes. :-)

::: toolkit/components/search/nsSearchSuggestions.js
@@ +32,3 @@
>      let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
> +    let label = bundle.GetStringFromName("suggestion_label");
> +    Object.defineProperty(SuggestAutoComplete, "_suggestionLabel", {value : label});

Nit: sorry, I forgot, also no space before the ":" :-)

Comment 9

[Prathik Sai [:Skandan]]

Attachment: Bug-1119682.diff

Comment 10

[Prathik Sai [:Skandan]]

Should I do something else? Or wait a bit more? Sorry if this sounds impatient. :)

Comment 11

[:Gijs (he/him)]

Comment on attachment 8547590 [details] [diff] [review]
Bug-1119682.diff

Matt, do you have more cycles to review this patch from Prathik?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8547590 [details] [diff] [review]
Bug-1119682.diff

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

::: toolkit/components/search/nsSearchSuggestions.js
@@ +32,3 @@
>      let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
> +    let label = bundle.GetStringFromName("suggestion_label");
> +    Object.defineProperty(SuggestAutoComplete, "_suggestionLabel", {value: label});

On comment 0 by Jeff, he suggests that this property should be defined on the prototype, and not directly on the object. The warning has gone away with this patch because _suggestionLabel is not redefined in this case and this code will run every time, I imagine.

Comment 13

[Prathik Sai [:Skandan]]

Attachment: Bug-1119682.diff(property added to prototype)

Sorry about that overlooked detail. Property added to prototype.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8547590 [details] [diff] [review]
Bug-1119682.diff

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

::: toolkit/components/search/nsSearchSuggestions.js
@@ +32,3 @@
>      let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
> +    let label = bundle.GetStringFromName("suggestion_label");
> +    Object.defineProperty(SuggestAutoComplete, "_suggestionLabel", {value: label});

Yeah.  You have <autocomplete object> -> ...some number of objects I don't know... -> SuggestAutoComplete.prototype -> Object.prototype -> null for the entire chain.  Only SAC.prototype had the property before, so delete did nothing, and assignment did nothing because the getter-only property made it a no-op.  With this patch you're defining onto SAC, which is not actually a member of the prototype chain, so the original getter will run every time.

One possibly more pleasant way to do this might be to move _suggestionLabel onto SuggestAutoComplete itself, entirely.

Object.defineProperty(SuggestAutoComplete, "_suggestionLabel",
{
  get() {
    delete this._suggestionLabel;
    let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
    return this._suggestionLabel = bundle.GetStringFromName("suggestion_label");
  }
});

and then change every |this._suggestionLabel| reference into a |SuggestAutoComplete._suggestionLabel| reference.  That's obviously more change than just modifying this one getter to do what it was intended to do, of course.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8549824 [details] [diff] [review]
Bug-1119682.diff(property added to prototype)

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

This approach as suggested in the beginning looks fine to me. Unless Waldo thinks there's some problem with it, this looks clean enough

Comment 16

[Jeff Walden [:Waldo]]

Nope, seems fine.

Comment 17

[Prathik Sai [:Skandan]]

So should I upload a new patch with the reviewer tag added and set checkin-needed or is there a way I can add the reviewer tag to the current patch itself?

Comment 18

[:Gijs (he/him)]

(In reply to Prathik Sai  [:Skandan] from comment #17)
> So should I upload a new patch with the reviewer tag added and set
> checkin-needed or is there a way I can add the reviewer tag to the current
> patch itself?

Normally, it'd be good to upload a new copy and set checkin-needed, yes. As it is, I just landed this for you:

remote:   https://hg.mozilla.org/integration/fx-team/rev/694b5bb3861f

Comment 19

[Prathik Sai [:Skandan]]

Thank You :Gijs :)

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/694b5bb3861f