The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla7

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 534922 [details] [diff] [review]
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
(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()".
(Assignee)

Comment 3

6 years ago
> 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.
(Assignee)

Comment 4

6 years ago
(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)
(Assignee)

Comment 5

6 years ago
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.
Attachment #534922 - Flags: review?(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?
Attachment #534922 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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.
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: fixed-in-cedar
Pushed:
http://hg.mozilla.org/mozilla-central/rev/39f04e61e994
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla7
(Assignee)

Comment 10

6 years ago
Pushed to aurora as test/comment-only change:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d7fbba849199
You need to log in before you can comment on or make changes to this bug.