1302991 - Empty url()s should not load anything

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Sebastian Zartner [:sebo]]

Attachment: Test case

CSS Values 3 says[1] this:

> If the value of the url() is the empty string (like url("") or url()), the url 
> must resolve to an invalid resource (similar to what the url about:invalid does).

Though Gecko currently causes a request for them, as shown in the attached test case.

Sebastian

[1] https://drafts.csswg.org/css-values-3/#url-empty

Comment 1

[Boris Zbarsky [:bzbarsky]]

The right fix here is probably to make css::URLValueData::GetURI() skip the NS_NewURI call if the string is null and hence treat the situation just like we treat NS_NewURI failing (i.e. invalid URL).

Comment 2

[Sumit Tiwari]

Attachment: Bug 1302991 - Empty url()s should not load or request anything

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

Comment 3

[Sumit Tiwari]

@Boris: It seems that I couldn't assign you as a reviewer because you aren't accepting any requests; please feel free to review it whenever you can. :)

Also, since I am not at all familiar with this part of the codebase, I am not entirely sure if the one check that I inserted will handle all the empty/null string cases. It does work for the attached testcase though (which is url() and not url("") or something else). We should probably lock this down using a test as well.

Comment 4

[Sebastian Zartner [:sebo]]

Comment on attachment 8805310 [details]
Bug 1302991 - Empty url()s should not load or request anything

David, as Boris currently doesn't accept review requests, could you please do a review for this? Or forward the request to someone else, so this code doesn't bitrot?

Comment 5

[David Baron :dbaron:]

Comment on attachment 8805310 [details]
Bug 1302991 - Empty url()s should not load or request anything

https://reviewboard.mozilla.org/r/89054/#review177634

Two surface concerns are:
 * it's better not to call NS_ConvertUTF16toUTF8(nsCSSValue::GetBufferValue(mString)) twice; it should instead be put in a variable.
 * mURIResolved should be set to true even if it's the empty string, so we don't keep executing the code

However, the bigger concern is what happens with stylo.  I'm not sure whether this code will still be used for this once we enable stylo (layout.css.servo.enabled).

There probably are also a few other cases that would need to be handled, since this code doesn't handle url() for @import rules or @namespace rules and maybe for some other case.

Comment 6

[David Baron :dbaron:]

Given how long this has sat already, I think it's best to come back to this in a month or two once stylo settles down.

Comment 7

[Sumit Tiwari]

Makes sense - thanks for the review David. Let's revisit this after the dust from Stylo has settled down.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Stylo has settled down.  Probably worth looking at this again.

Sumit, are you still in a position to work on this?

Comment 9

[Sebastian Zartner [:sebo]]

Sumit obviously wasn't active on Bugzilla for more than a year and didn't answer to Boris' request, therefore I reset the assignee now.

Sebastian

Comment 10

[Sebastian Zartner [:sebo]]

Stylo has settled for quite some time now, so maybe this could be reprioritized now.

For what it's worth, there was an issue mentioned on Stack Overflow lately, which was caused by this.

Furthermore, Chromium has the same issue and also has a bug filed for it.

Sebastian

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1302991 - Move the check to prevent loading internal resources as images. r=#layout-reviewers

No behavior change, but the new place seems more appropriate.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1302991 - Do not try to resolve empty CSS url values. r=#layout-reviewers

So as to not try to load the same document as a subresource.

Depends on D103716

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd4ba1eeec00
Move the check to prevent loading internal resources as images. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e4734e2b38b6
Do not try to resolve empty CSS url values. r=dholbert

Comment 14

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27472 for changes under testing/web-platform/tests

Comment 15

[Atila Butkovits]

Backed out for causing failures on empty.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/45de327a70b9ffd1beb3c0cde2a551ffee58b0c7

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=J3Oci86oRMuvqSFujCrCsg.0&revision=e4734e2b38b6472151c3ae2945073ed8dd5cbc95&searchStr=windows%2C10%2Cx64%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-windows10-64%2Fopt-web-platform-tests-e10s%2Cwpt4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328802766&repo=autoland&lineNumber=3814

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Broken test.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5785ba1a0aba
Move the check to prevent loading internal resources as images. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a2e7db8b9e98
Do not try to resolve empty CSS url values. r=dholbert

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5785ba1a0aba
https://hg.mozilla.org/mozilla-central/rev/a2e7db8b9e98

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot