1301854 - CSS with UTF-16LE shown as raw bytes in CSS Rules Inspector

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Mehdi]

Attachment: 1.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

The CSS has bundled with UTF-8 unicode.

Please note: other files's CSS shown correctly.


Actual results:

CSS rules shown as decoded characters


Expected results:

Show the correct characters

Comment 1

[Tooru Fujisawa [:arai]]

Can you provide a testcase?

Comment 2

[Mehdi]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> Can you provide a testcase?

Here you go (https://github.com/dehghani-mehdi/firefox-dev-testcase)

Comment 3

[Mehdi]

You can find the testcase, here:
https://github.com/dehghani-mehdi/firefox-dev-testcase

Comment 4

[Tooru Fujisawa [:arai]]

thanks :)

the CSS file is using UTF-16LE, and it's shown as raw bytes there.

Here's regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f354f556b59136bc052e5b2c57d7d28e7242de37&tochange=40f709d9c088dfc4ba1b55fb80920b5ff5102842

looks like it's from bug 984880.

Comment 5

[Loic]

Attachment: bundle.css

Comment 6

[Loic]

Attachment: index.html

Comment 7

[Mehdi]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> thanks :)
> 
> the CSS file is using UTF-16LE, and it's shown as raw bytes there.
> 
> Here's regression range:
> https://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=f354f556b59136bc052e5b2c57d7d28e7242de37&tochange=40f7
> 09d9c088dfc4ba1b55fb80920b5ff5102842
> 
> looks like it's from bug 984880.

NP :]

I'm sorry, but I didn't get your point.
Is this (I call it bug) fixed? or will be fixed?

Other browsers have not any problem with that bundle, incl. Firefox. I saw this problem only in Firefox Dev Edition.

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Mehdi from comment #7)
> Is this (I call it bug) fixed? or will be fixed?

the change in bug 984880 introduced this issue, and the issue is not yet fixed.

Comment 9

[Tom Tromey :tromey]

This looks like another instance of bug 118268 - bundle.css has a BOM, but for some
reason the code in StyleSheetActor._getCSSCharset doesn't recognize it.
If I add a charset to the <link>, like <link href="bundle.css" rel="stylesheet" charset="utf-16">,
then it works for me.

Comment 10

[Patrick Brosset <:pbro>]

This sounds really bad. Triaging as P1. Tom do you think P1 is right for this bug? Is this something that might happen frequently enough? Or is this an edge case?

Comment 11

[Tom Tromey :tromey]

(In reply to Patrick Brosset <:pbro> from comment #10)
> This sounds really bad. Triaging as P1. Tom do you think P1 is right for
> this bug? Is this something that might happen frequently enough? Or is this
> an edge case?

I don't know whether it's common to use a BOM or a non-UTF-8 character set for
CSS.  I also don't know whether any data on this is available.  So, I don't know, sorry.

Comment 12

[Tom Tromey :tromey]

The bug happens because this code:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#458
... doesn't do any BOM sniffing.
I have a patch.

Comment 14

[Tom Tromey :tromey]

Attachment: Bug 1301854 - detect BOM when determining resource encoding;

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

Comment 15

[Tom Tromey :tromey]

Oops, I left in some code from an earlier attempt using BinaryInputStream.

Comment 16

[Tom Tromey :tromey]

... and attached the wrong version besides.  twofer.

Comment 17

[Tom Tromey :tromey]

Comment on attachment 8792088 [details]
Bug 1301854 - detect BOM when determining resource encoding;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79318/diff/1-2/

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8792088 [details]
Bug 1301854 - detect BOM when determining resource encoding;

https://reviewboard.mozilla.org/r/79318/#review78464

::: devtools/client/styleeditor/test/browser.ini:60
(Diff revision 2)
>    !/devtools/client/inspector/shared/test/head.js
>    !/devtools/client/inspector/test/head.js
>    !/devtools/client/inspector/test/shared-head.js
>    !/devtools/client/shared/test/test-actor-registry.js
>    !/devtools/client/shared/test/test-actor.js
> +  utf-16.css

nit: I think we want to keep the !/devtools/client/... lines at the end  of the support-files block. So you can move the utf-16.css line right after the sync.html line.

::: devtools/shared/DevToolsUtils.js:451
(Diff revision 2)
> +      // We do our own BOM sniffing here because there's no convenient
> +      // implementation of the "decode" algorithm
> +      // (https://encoding.spec.whatwg.org/#decode) exposed to JS.
> +      let bomCharset = null;
> +      if (available >= 3 && source.codePointAt(0) == 0xef &&
> +          source.codePointAt(1) == 0xbb && source.codePointAt(2) == 0xbf) {
> +        bomCharset = "UTF-8";
> +        source = source.slice(3);
> +      } else if (available >= 2 && source.codePointAt(0) == 0xfe &&
> +                 source.codePointAt(1) == 0xff) {
> +        bomCharset = "UTF-16BE";
> +        source = source.slice(2);
> +      } else if (available >= 2 && source.codePointAt(0) == 0xff &&
> +                 source.codePointAt(1) == 0xfe) {
> +        bomCharset = "UTF-16LE";
> +        source = source.slice(2);
> +      }

I'll have to trust you on this :) as I have no knowledge whatsoever about how this works.
The code looks Ok to me, and the issue is solved, so I'm willing to R+, but if you think you need some more expert eyes on this piece of code, I suggest asking someone else.

Comment 19

[Tom Tromey :tromey]

Comment on attachment 8792088 [details]
Bug 1301854 - detect BOM when determining resource encoding;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79318/diff/2-3/

Comment 20

[Tom Tromey :tromey]

Comment on attachment 8792088 [details]
Bug 1301854 - detect BOM when determining resource encoding;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79318/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33304c191afb
detect BOM when determining resource encoding; r=pbro

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33304c191afb

Comment 23

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on firefox nightly according to(2016-09-09)

Fixing bug is verified on Latest Nightly--Build ID:(20161030030204),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

now it is correctly running on latest nightly.

Tested OS-- Windows10 32bit

Comment 24

[Petruta Horea [:phorea], Desktop QA]

Thanks for verifying, :Antora!

I also checked that this is fixed using latest Nightly 52.0a1 under Mac OS X 10.11. Marking as verified.

Comment 25

[Mehdi]

Hi,

This issue doesn't fix yet!, why the status is updated to FIXED?

Comment 26

[Loic]

(In reply to Mehdi from comment #25)
> Hi,
> 
> This issue doesn't fix yet!, why the status is updated to FIXED?

It's fixed in FF52+, read the flags on the right.