659466 - Fix remaining "#-in-data-URI" issues in test files

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 658949 (intentionally) broke data URIs that have "#" in them, basically.

"patch 0" on that bug fixed all the test failures that resulted from this.  I'm now doing an audit for remaining instances of this (that aren't causing test failures for whatever reason).  I don't anticipate that there will be many of these, but I've already found a few.*

FWIW, I'm finding these by running
> grep -r -A6 "data:"
across m-c, and then searching through the results for instances of "#".  Hopefully this is sensitive enough to catch most or all instances of this.

* so far, I've found this in window_activation.xul & test_activation.xul

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's the fix for all the instances I found.  Haven't tested it yet.

Notes on the touched files & why they weren't failing after bug 658949 landed:
=====
> dom/tests/mochitest/chrome/test_activation.xul
> dom/tests/mochitest/chrome/window_activation.xul

(still need to look into these two xul files)

> gfx/thebes/gfxFont.cpp

Just fixing a sample data URI given in a comment.

> layout/reftests/bugs/605138-1-ref.html
> layout/reftests/bugs/605138-1.html

In this case, bug 658949 broke the same data URI in the testcase & reference case, so they continued rendering the same.  Makes sense.

> layout/style/test/test_bug405818.html

In this case, the test just does a string-comparison on some data URIs that have # characters in them, which still works fine even after bug 658949 busted the semantics of those URIs.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> =====
> > dom/tests/mochitest/chrome/test_activation.xul
> > dom/tests/mochitest/chrome/window_activation.xul
> 
> (still need to look into these two xul files)

(note: window_activation.xul is a helper file for test_activation.xul.)

Hmm, I see... this test wasn't affected because it doesn't actually check anything right now. :)

The test currently has lines 4 lines like this:
> ok(getComputedStyle(document.getElementById("box"), "").backgroundColor, "rgb(0, 0, 255)");

Note that the above line uses "ok()", which just evaluates the truthiness of its first argument.  So in current mozilla-central, as long as .backgroundColor is *anything* truthy, the test passes.

It wants to be using "is()", not "ok()".

Comment 3

[Daniel Holbert [:dholbert]]

> It wants to be using "is()", not "ok()".

...and once I fix that (and apply this bug's hash-fixing-patch), the test fails.

I'll file a separate bug on that, after making absolutely sure it's unrelated to this hash-in-data-URI business.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> I'll file a separate bug on that
(filed bug 659522 on fixing up the ineffective checks in window_activation.xul)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 534922 [details] [diff] [review]
fix

Requesting review.  See comment 1 & 2 for an overview of the changed files and why the existing testcases aren't currently causing orange.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534922 [details] [diff] [review]
fix

r=me

The fact that some of these tests were passing is sort of sadmaking.  :(  Any way we can change them to fail in the future if people do this sort of thing?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> Comment on attachment 534922 [details] [diff] [review] [review]
> fix
> 
> r=me
> 
> The fact that some of these tests were passing is sort of sadmaking.

yeah :-/ (just to clarify: _all_ the tests on _this_ bug here were passing)

> Any way we can change them to fail in the future if people do this sort of thing?

The *_activation.xul tests are just broken, and once they're fixed up, they'll presumably depend on getting colors from their stylesheet in order to pass. Bug 659522 covers that.

I'll see about adding some minor bit of color to the data-URI-stylesheets in 605138-1.html.

And I suppose we could examine the computed style of some element in test_bug405818.html and assert that it's what we expect.

Comment 8

[Daniel Holbert [:dholbert]]

Landed fix in cedar: http://hg.mozilla.org/projects/cedar/rev/39f04e61e994

I added an 'outline' declaration to the data-URI-stylesheet in 605138-1.html, whereas in the reference case, I added this declaration inside a new <style> chunk.  So now if the data URI fails to parse correctly, that test will fail.

I also added a computed-style check to test_bug405818.html to be sure that the data-URI-stylesheet there loads correctly.

Comment 9

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/39f04e61e994

Comment 10

[Daniel Holbert [:dholbert]]

Pushed to aurora as test/comment-only change:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d7fbba849199