Open Bug 1302991 Opened 4 years ago Updated 5 months ago
Empty url()s should not load anything
CSS Values 3 says 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  https://drafts.csswg.org/css-values-3/#url-empty
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).
@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 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?
Assignee: nobody → sumi29
Status: NEW → ASSIGNED
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.
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.
Makes sense - thanks for the review David. Let's revisit this after the dust from Stylo has settled down.
Stylo has settled down. Probably worth looking at this again. Sumit, are you still in a position to work on this?
Assignee: sumi29 → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.