1105099 - JavaScript strict warning: resource:///modules/DirectoryLinksProvider.jsm, line 399: ReferenceError: reference to undefined property link.enhancedImageURI

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, defect)

Description

[Daniel Holbert [:dholbert]]

Noticed this go by in my debug-spew when starting Firefox (debug build, fresh profile):
{
JavaScript strict warning: resource:///modules/DirectoryLinksProvider.jsm, line 399: ReferenceError: reference to undefined property link.enhancedImageURI
}

Code quote:
> 395       return rawLinks.filter(link => {
> 396         // Make sure the link url is allowed and images too if they exist
> 397         return this.isURLAllowed(link.url, ALLOWED_LINK_SCHEMES) &&
> 398                this.isURLAllowed(link.imageURI, ALLOWED_IMAGE_SCHEMES) &&
> 399                this.isURLAllowed(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES);

Looks like this line was added in bug 1088729.

Mardak, should we be sanity-checking for a missing "enhancedImageURI" here? (Or should all 'link' variables here have a defined enhancedImageURI, in which case this is a busted 'link' object?)

Comment 1

[Ed Lee :Mardak]

Not all link objects have an enhancedImageURI and isURLAllowed short circuits to true if there's no value. Not sure what's the common pattern to avoid having that warning trigger.

Comment 2

[Daniel Holbert [:dholbert]]

I'm not sure what the common pattern is either, but based on MDN documentation, it looks like "in" should work relatively elegantly:
   https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in

so I think we'd have e.g.:
   this.isURLAllowed("enhancedImageURI" in link ? link.enhancedImageURI : null,
                     ALLOWED_IMAGE_SCHEMES)

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Reading out values with let {} destructuring seems to not trigger the warning.

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

Or one fewer total lines by destructuring in the function parameters.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8528779 [details] [diff] [review]
v1.1

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

I think link.enhancedImageURI || null would work too.

Comment 6

[Ed Lee :Mardak]

Attachment: v2

(In reply to Drew Willcoxon :adw from comment #5)
> I think link.enhancedImageURI || null would work too.
Oh! You are indeed quite right. Using that instead as it makes things more explicit that we can have an optional enhancedImageURI.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8529188 [details] [diff] [review]
v2

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

OK.  To be clear, I liked both your previous patches too, but I'll defer to you to choose the one you like best. :-)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Ed Lee :Mardak from comment #6)
> > I think link.enhancedImageURI || null would work too.
> Oh! You are indeed quite right. Using that instead as it makes things more
> explicit that we can have an optional enhancedImageURI.

Are you sure that actually silences the warning? I'd assume that would still count as a "reference to undefined property" (since we'd still be trying to grab the property, before we know if it's there).

The second-to-last example in this stackoverflow question...
  https://stackoverflow.com/questions/3789277/javascript-reference-to-undefined-property
...increases my suspicion about that. (The example is: "if (!mySidebar.context.netProgress)", and netProgress isn't defined, and the questioner says that triggers a warning.)

Comment 9

[Drew Willcoxon :adw]

It does.  The rules of strict mode are pretty abstruse. :-/

Comment 10

[Jessilyn Davis]

Hello!

This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience.

Please help us triage by reopening if this issue still persists and should be addressed.

Thanks!