Closed Bug 1031942 Opened 10 years ago Closed 10 years ago

Newtab thumbnails missing if URL contains unusual characters

Categories

(Firefox :: New Tab Page, defect)

30 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: marc, Assigned: marc)

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611060256

Steps to reproduce:

The newtab thumbnail for the following URL is not displayed:
http://en.wikipedia.org/wiki/Tool_(band)#Hiatus_and_fifth_studio_album_.282008.E2.80.93present.29

The thumbnail image is present in the cache folder. I've tracked the bug down to the "refreshThumbnail" function in browser/base/content/newtab/sites.js. The uri needs to be wrapped in an "encodeURIComponent" call to produce a valid thumbnail URL from the URL above.


Actual results:

Thumbnail is not displayed.
Message "Error in parsing value for 'background-image'.  Declaration dropped." in the error console.
Thumbnails for other pages (with simpler URLs) are shown correctly.
Can you still reproduce this in a more recent version of Firefox? I don't see this behaviour in Nightly or Aurora (http://nightly.mozilla.org/ , http://aurora.mozilla.org/ ) - do you?
Component: Untriaged → New Tab Page
Flags: needinfo?(marc)
(In reply to :Gijs Kruitbosch from comment #1)
> Can you still reproduce this in a more recent version of Firefox? I don't
> see this behaviour in Nightly or Aurora (http://nightly.mozilla.org/ ,
> http://aurora.mozilla.org/ ) - do you?

Yes, reproduced with the latest nightly.
Flags: needinfo?(marc)
Attached image ff_bug.png
Screenshot of the bug in the latest nightly (note the blank tile).
Fiddling with the developer console a little more, it turns out that the problem are the parenthesis in the URL together with the fact that encodeURIComponent does not escape them.

It seems that all that is actually needed is enclosing the URL in extra quotation marks so that the url() syntax is not confused by the parens.
Attached patch sites.js.diff (obsolete) — Splinter Review
Proposed change in sites.js.
Hm, I can reproduce now. Don't know why I couldn't before.

It's somewhat odd because in one of the paths called from sites.js (the PageThumbs one) we do encodeURIComponent the URL... Drew, can you look into this in more detail, and/or review the patch? :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(adw)
OS: Linux → All
Hardware: x86 → All
Thanks for the patch.  I think we should flip the single and double quotes, like:

    thumbnail.style.backgroundImage = 'url("' + uri + '")';

encodeURIComponent escapes double quotes but not single quotes, so if we use single quote delimeters, we'll still be tripped up by URLs with single quotes in them.

Could you please attach a new patch?

I actually didn't know there was a difference between quoted and unquoted URIs, but it makes sense.  For reference: http://dev.w3.org/csswg/css-values/#urls
Flags: needinfo?(adw)
Attached patch sites.js.diff (obsolete) — Splinter Review
New patch with flipped quotes as suggested in comment #7.
Attachment #8448128 - Attachment is obsolete: true
Comment on attachment 8448884 [details] [diff] [review]
sites.js.diff

Great, thank you.
Attachment #8448884 - Flags: review+
Assignee: nobody → marc
Status: NEW → ASSIGNED
Keywords: checkin-needed
Proper patch, change is identical to the previous one.
Attachment #8448884 - Attachment is obsolete: true
Attachment #8448949 - Flags: review+
Hi,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dac746a07728
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Thanks for getting this committed so quickly!
I have tried getting bugs fixed/patches approved in other projects before, and I have to say it's awesome how responsive you guys are.
QA Whiteboard: [good first verify]
I could try it, after release of beta 33, if it will be ok.
Flags: needinfo?(lars.gusowski+bugzilla)
Lars, if you want you can also use the first Beta build of Firefox 33 (ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/33.0b1-candidates/build1/).
for me it is fixed:
User-Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Status change is not possible for me as i see.
Flags: needinfo?(lars.gusowski+bugzilla)
Thank you, Lars.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: