Closed Bug 1105099 Opened 7 years ago Closed 2 years ago

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

Categories

(Firefox :: New Tab Page, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dholbert, Assigned: Mardak)

References

Details

Attachments

(1 file, 2 obsolete files)

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?)
Flags: needinfo?(edilee)
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.
Flags: needinfo?(edilee)
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)
Attached patch v1 (obsolete) — Splinter Review
Reading out values with let {} destructuring seems to not trigger the warning.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8528777 - Flags: review?(adw)
Attached patch v1.1 (obsolete) — Splinter Review
Or one fewer total lines by destructuring in the function parameters.
Comment on attachment 8528779 [details] [diff] [review]
v1.1

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

I think link.enhancedImageURI || null would work too.
Attachment #8528779 - Flags: review+
Attachment #8528777 - Flags: review?(adw)
Attached patch v2Splinter Review
(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.
Attachment #8528777 - Attachment is obsolete: true
Attachment #8528779 - Attachment is obsolete: true
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. :-)
Attachment #8529188 - Flags: review+
(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.)
It does.  The rules of strict mode are pretty abstruse. :-/

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!

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.