1312216 - Strict warning: ReferenceError: reference to undefined property this._openedURL

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P5)

Description

[Richard Marti (:Paenglab)]

When I open a subdialog I get this error:
19:58:19.849 ReferenceError: reference to undefined property this._openedURL 1 subdialogs.js:46:1

Comment 1

[:Gijs (he/him)]

That's a strict warning, despite the "ReferenceError" warning, I'm pretty sure. Everything is working correctly.

We could explicitly set the value to null and reset to null instead of calling delete;. That would silence this warning.

Comment 2

[Alan]

(In reply to :Gijs from comment #1)
> That's a strict warning, despite the "ReferenceError" warning, I'm pretty
> sure. Everything is working correctly.
> 
> We could explicitly set the value to null and reset to null instead of
> calling delete;. That would silence this warning.

I assume from what you have said this is the line causing issues:
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/subdialogs.js#115

From what I read online and saw in the code the issue is caused because we are trying to delete this._openedURL and if it is ever used again it causes the error?

A fix would be to:
Set this._openedURL to null instead of calling delete.

Comment 3

[Alan]

(In reply to Richard Marti (:Paenglab) from comment #0)
> When I open a subdialog I get this error:
> 19:58:19.849 ReferenceError: reference to undefined property this._openedURL
> 1 subdialogs.js:46:1

What steps did you take to produce this error?

Comment 4

[:Gijs (he/him)]

(In reply to Alan from comment #2)
> (In reply to :Gijs from comment #1)
> > That's a strict warning, despite the "ReferenceError" warning, I'm pretty
> > sure. Everything is working correctly.
> > 
> > We could explicitly set the value to null and reset to null instead of
> > calling delete;. That would silence this warning.
> 
> I assume from what you have said this is the line causing issues:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/in-content/subdialogs.js#115

Yes. Using this._openedURL = null; instead would fix this.

> From what I read online and saw in the code the issue is caused because we
> are trying to delete this._openedURL and if it is ever used again it causes
> the error?

Yeah, on debug builds the JS engine logs warnings when you rely on a property's value if that property does not exist. We delete the property, but if we set it to null instead that will shut up the JS engine's warnings (because the property will always exist - it'll just be null some of the time.

(In reply to Alan from comment #3)
> (In reply to Richard Marti (:Paenglab) from comment #0)
> > When I open a subdialog I get this error:
> > 19:58:19.849 ReferenceError: reference to undefined property this._openedURL
> > 1 subdialogs.js:46:1
> 
> What steps did you take to produce this error?

I expect you need a debug build, and then open the preferences/options, and e.g. go to the "content" section and click a button that shows a dialog, like the "Exceptions..." one next to popups. You might need to close and reopen to see this error, I'm not sure off-hand.

Comment 5

[Richard Marti (:Paenglab)]

Open FX, then the prefs.
Open and clear JS console.
Go to Content tab and open a subdialog (e. g. Colors).
Error appears only the first time. You have to close the prefs to see the error again on opening a dialog. For me it appears on every sub dialog.

But like Gijs wrote, you have to set javascript.options.strict to true to see the error.

Comment 6

[Richard Marti (:Paenglab)]

No debug build needed (I never use one). Only set javascript.options.strict to true.

Comment 7

[Alan]

So I made the change to the subdialogs.js file, however, I have a lot of @TOPSCRDIR@ files that I committed. Now when I "hg push review" the searching for changes part is taking hours so I have been able to hg push review correctly. Can someone explain what I did wrong?

Comment 8

[Alan]

Attachment: bug

Comment 9

[:Gijs (he/him)]

(In reply to Alan from comment #7)
> So I made the change to the subdialogs.js file, however, I have a lot of
> @TOPSCRDIR@ files that I committed. Now when I "hg push review" the
> searching for changes part is taking hours so I have been able to hg push
> review correctly. Can someone explain what I did wrong?

Hey Alan, what specifically do you mean when you say you have a lot of committed @TOPSRCDIR@ files? If you want to remove those commits, you can likely pass the revision of the broken commit (with -r ) to either 'hg prune' or 'hg strip' to do so (though be careful as this permanently removes the revisions in question from your local repository).

Comment 10

[Alan]

(In reply to :Gijs from comment #9)
> (In reply to Alan from comment #7)
> > So I made the change to the subdialogs.js file, however, I have a lot of
> > @TOPSCRDIR@ files that I committed. Now when I "hg push review" the
> > searching for changes part is taking hours so I have been able to hg push
> > review correctly. Can someone explain what I did wrong?
> 
> Hey Alan, what specifically do you mean when you say you have a lot of
> committed @TOPSRCDIR@ files? If you want to remove those commits, you can
> likely pass the revision of the broken commit (with -r ) to either 'hg
> prune' or 'hg strip' to do so (though be careful as this permanently removes
> the revisions in question from your local repository).

I miss spoke about the TOPSRCDIR files. But when I am trying to "hg push review" it's taking ~2 hours. I am thinking about cloning the repo again. What do you think is the best? Yeah I read about strip and prune.

Comment 11

[:Gijs (he/him)]

(In reply to Alan from comment #10)
> (In reply to :Gijs from comment #9)
> > (In reply to Alan from comment #7)
> > > So I made the change to the subdialogs.js file, however, I have a lot of
> > > @TOPSCRDIR@ files that I committed. Now when I "hg push review" the
> > > searching for changes part is taking hours so I have been able to hg push
> > > review correctly. Can someone explain what I did wrong?
> > 
> > Hey Alan, what specifically do you mean when you say you have a lot of
> > committed @TOPSRCDIR@ files? If you want to remove those commits, you can
> > likely pass the revision of the broken commit (with -r ) to either 'hg
> > prune' or 'hg strip' to do so (though be careful as this permanently removes
> > the revisions in question from your local repository).
> 
> I miss spoke about the TOPSRCDIR files. But when I am trying to "hg push
> review" it's taking ~2 hours. I am thinking about cloning the repo again.
> What do you think is the best? Yeah I read about strip and prune.

I don't know why that would happen. Are you passing a revision to push (so something like "hg push -r . review"), and if not, does passing a rev go any faster?

If that doesn't help, you can also use "hg export" to create a patch file and just attach it to the bug. It looks like you already did that, but the commit you attached doesn't have the actual change, but seems to be on top of the earlier change (from 'delete this._openedURL' to 'this._openedURL = null')

Comment 12

[Alan]

Attachment: Bug 1312216 - Changed delete this._openedURL and set it as null instead. There was a strict warning.

Review commit: https://reviewboard.mozilla.org/r/105324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105324/

Comment 13

[Alan]

(In reply to :Gijs from comment #11)
> (In reply to Alan from comment #10)
> > (In reply to :Gijs from comment #9)
> > > (In reply to Alan from comment #7)
> > > > So I made the change to the subdialogs.js file, however, I have a lot of
> > > > @TOPSCRDIR@ files that I committed. Now when I "hg push review" the
> > > > searching for changes part is taking hours so I have been able to hg push
> > > > review correctly. Can someone explain what I did wrong?
> > > 
> > > Hey Alan, what specifically do you mean when you say you have a lot of
> > > committed @TOPSRCDIR@ files? If you want to remove those commits, you can
> > > likely pass the revision of the broken commit (with -r ) to either 'hg
> > > prune' or 'hg strip' to do so (though be careful as this permanently removes
> > > the revisions in question from your local repository).
> > 
> > I miss spoke about the TOPSRCDIR files. But when I am trying to "hg push
> > review" it's taking ~2 hours. I am thinking about cloning the repo again.
> > What do you think is the best? Yeah I read about strip and prune.
> 
> I don't know why that would happen. Are you passing a revision to push (so
> something like "hg push -r . review"), and if not, does passing a rev go any
> faster?
> 
> If that doesn't help, you can also use "hg export" to create a patch file
> and just attach it to the bug. It looks like you already did that, but the
> commit you attached doesn't have the actual change, but seems to be on top
> of the earlier change (from 'delete this._openedURL' to 'this._openedURL =
> null')

Finally was able to get my repo working! 'hg roots(outgoing())' got rid of my outgoing changes and I did a hg update --clean. Thanks for the help with everything Gijs I really appreciate it.

Comment 14

[:Gijs (he/him)]

Great! So now, we need to get someone to review it. If you make the commit message end in ", r?gijs" and re-push it (you can use "hg commit --amend" to adjust the last commit, and then just use 'hg push -r . review' again), it'll automatically ask me for review.

(I would have set it in reviewboard myself, but it's telling me you have a draft in addition to the now public changeset, and so it won't let me change it while there's a new unpublished draft revision...)

Comment 15

[Alan]

(In reply to :Gijs from comment #14)
> Great! So now, we need to get someone to review it. If you make the commit
> message end in ", r?gijs" and re-push it (you can use "hg commit --amend" to
> adjust the last commit, and then just use 'hg push -r . review' again),
> it'll automatically ask me for review.
> 
> (I would have set it in reviewboard myself, but it's telling me you have a
> draft in addition to the now public changeset, and so it won't let me change
> it while there's a new unpublished draft revision...)

I think I forgot to publish haha.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8827699 [details]
Bug 1312216 - Changed delete this._openedURL and set it as null instead. There was a strict warning.

https://reviewboard.mozilla.org/r/105324/#review106454

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/580064ea1d81
Changed delete this._openedURL and set it as null instead. There was a strict warning. r=Gijs

Comment 18

[:Gijs (he/him)]

(In reply to Alan from comment #15)
> (In reply to :Gijs from comment #14)
> > Great! So now, we need to get someone to review it. If you make the commit
> > message end in ", r?gijs" and re-push it (you can use "hg commit --amend" to
> > adjust the last commit, and then just use 'hg push -r . review' again),
> > it'll automatically ask me for review.
> > 
> > (I would have set it in reviewboard myself, but it's telling me you have a
> > draft in addition to the now public changeset, and so it won't let me change
> > it while there's a new unpublished draft revision...)
> 
> I think I forgot to publish haha.

Fabulous, this works - thanks!

Comment 19

[Richard Marti (:Paenglab)]

I still get the error with the patch applied.

Shouldn't it be a _openedURL: null, after var gSubDialog = { to define _openedURL initially?

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/580064ea1d81

Comment 21

[:Gijs (he/him)]

(In reply to Richard Marti (:Paenglab) from comment #19)
> I still get the error with the patch applied.
> 
> Shouldn't it be a _openedURL: null, after var gSubDialog = { to define
> _openedURL initially?

D'oh, I should have caught this in review. Followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ac20f11d85

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/92ac20f11d85

Comment 23

[:Gijs (he/him)]

Richard, can you confirm this is fixed now?

Comment 24

[Richard Marti (:Paenglab)]

Yes, this is fixed now.
Thank you Alan and Gijs.