1031942 - Newtab thumbnails missing if URL contains unusual characters

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[MSchmitzer]

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.

Comment 1

[:Gijs (he/him)]

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?

Comment 2

[MSchmitzer]

(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.

Comment 3

[MSchmitzer]

Attachment: ff_bug.png

Screenshot of the bug in the latest nightly (note the blank tile).

Comment 4

[MSchmitzer]

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.

Comment 5

[MSchmitzer]

Attachment: sites.js.diff

Proposed change in sites.js.

Comment 6

[:Gijs (he/him)]

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? :-)

Comment 7

[Drew Willcoxon :adw]

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

Comment 8

[MSchmitzer]

Attachment: sites.js.diff

New patch with flipped quotes as suggested in comment #7.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8448884 [details] [diff] [review]
sites.js.diff

Great, thank you.

Comment 10

[MSchmitzer]

Attachment: bug_1031942.patch

Proper patch, change is identical to the previous one.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[:Gijs (he/him)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=ac87cc0a01ff

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/dac746a07728

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dac746a07728

Comment 15

[MSchmitzer]

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.

Comment 16

[Lars Gusowski [:lagu]]

I could try it, after release of beta 33, if it will be ok.

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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/).

Comment 18

[Lars Gusowski [:lagu]]

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.

Comment 19

[Drew Willcoxon :adw]

Thank you, Lars.