Last Comment Bug 683710 - Should probably strip comments from shader sources before validating bad character and passing to the GL
: Should probably strip comments from shader sources before validating bad char...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
http://demos.vicomtech.org/volren/
Depends on:
Blocks: webgl-conformance 680722
  Show dependency treegraph
 
Reported: 2011-08-31 13:48 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2011-09-22 17:06 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, shader source comment validation fix. (8.86 KB, patch)
2011-09-01 18:01 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Comment stripping speed test (2.66 KB, text/html)
2011-09-01 18:49 PDT, Doug Sherk (:drs) (inactive)
no flags Details
Patch v1.0, shader source comment validation fix, with optimizations. (9.08 KB, patch)
2011-09-01 19:05 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.1, shader source comment validation fix. (13.38 KB, text/plain)
2011-09-02 16:11 PDT, Doug Sherk (:drs) (inactive)
no flags Details
Patch v1.2, shader source comment validation fix. (17.00 KB, patch)
2011-09-02 19:01 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch v1.2, shader source comment validation fix. (17.42 KB, patch)
2011-09-06 14:38 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v1.3, shader source comment validation fix. (24.43 KB, patch)
2011-09-06 18:12 PDT, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Splinter Review
address Neil's string comments (2.75 KB, patch)
2011-09-18 22:08 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review? (neil)
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-31 13:48:00 PDT
This demo:
  http://demos.vicomtech.org/volren/

doesn't work anymore, with this warning:

Warning: WebGL: shaderSource: string contains the illegal character '34'
Source File: http://demos.vicomtech.org/volren/volumerc.js
Line: 72

The reason is that it has a comment containing the " character (ASCII 34) which is one of the illegal characters we're now rejecting in shader sources.

See bug 680722.

We need to do like WebKit which, in my understanding, strips comments out of shader sources before validating against bad characters and before passing the string to the GL.

See the StripComments class here:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp?rev=93625#L139

We probably should borrow that code and give appropriate credit, rather than try to reinvent the wheel.
Comment 1 Doug Sherk (:drs) (inactive) 2011-08-31 17:07:30 PDT
Copying the WebKit code seemed like a good idea to me on first glance, but after trying it, it's a total mess of dependencies to objects that we don't have good replacements for. For now, I think it's a better idea to rewrite this. I'm going to get started on it, although I suspect that I'll be pulled to do something else before I make significant progress on it.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2011-08-31 20:04:18 PDT
I had a look at their code an clearly a lot of thought has gone into it: preservation of newlines so that line numbers remain meaningful, preservation of certain preprocessor directives.

The only thing that will need to be adapted, as far as I can see, is the strings API. They have a neat string API and we need to port that to our sucky nsString.

What other dependencies did you see?
Comment 3 Doug Sherk (:drs) (inactive) 2011-08-31 21:42:21 PDT
Ok, I figured porting over their strings was off limits because of how relatively massive that collection of code would be. There are a ton of other dependencies, most of which are easy to resolve (like UChar, stuff like that). I'm also not aware of what the "WTF" set of headers are exactly, other than a funny name.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-09-01 11:07:12 PDT
Of course we don't want to port their strings classes; instead we want to port this parser to use our own string classes, **** as they are.
Comment 5 Doug Sherk (:drs) (inactive) 2011-09-01 18:01:04 PDT
Created attachment 557726 [details] [diff] [review]
Patch v1.0, shader source comment validation fix.

This code is copied mostly from WebKit. It strips out comments from shader source code before actually compiling it, so that the comments can have illegal characters. It was benchmarked and it was noted that a test attached to the bug ticket took about twice as long with these changes.
Comment 6 Doug Sherk (:drs) (inactive) 2011-09-01 18:49:43 PDT
Created attachment 557736 [details]
Comment stripping speed test

Takes ~8.5s according to Instruments without comment stripping, ~16.5s with.
Comment 7 Doug Sherk (:drs) (inactive) 2011-09-01 19:05:20 PDT
Created attachment 557742 [details] [diff] [review]
Patch v1.0, shader source comment validation fix, with optimizations.

This is a version of the previous patch with two trivial optimizations which cut down the runtime of comment-stripping-speed-test.html from ~16.5s to ~15.5s according to my benchmarks. I don't actually believe these are worth applying because they make the code more confusing with little performance improvement.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-09-01 19:46:10 PDT
Comment on attachment 557726 [details] [diff] [review]
Patch v1.0, shader source comment validation fix.

Review of attachment 557726 [details] [diff] [review]:
-----------------------------------------------------------------

Good work; some comments:

::: content/canvas/src/WebGLContextGL.cpp
@@ +4133,5 @@
>      WebGLuint shadername;
>      if (!GetConcreteObjectAndGLName("shaderSource: shader", sobj, &shader, &shadername))
>          return NS_OK;
>  
> +    nsString cleanSource = StripComments(nsString(source)).result();

Why don't you declare cleanSource as a const nsString& so that you avoid a useless deep copy.

::: content/canvas/src/WebGLValidateStrings.h
@@ +35,2 @@
>  // which can be found here:
>  // http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp#L123

When pointing to a precise line number, it's best to link to a precise revision number otherwise the line number end up dangling.

@@ +35,3 @@
>  // which can be found here:
>  // http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp#L123
> +// Note that some modifications were done to adapt it to the Mozilla Core API.

I'd just say "adapt it to Mozilla" or some such, as "Core API" is not a very common phrase around here. You could also say "strings API", I don't know.

@@ +66,5 @@
> +        {
> +            parse();
> +        }
> +
> +        nsString result()

return a const nsString& here, so you don't make a deep-copy of the string ;-)

I don't know how WebKit's String class works, maybe it has copy-on-write as after all WebKit comes from the KDE world which is addicted to copy-on-write but even so it's still a better idea to return a const reference to avoid doing any unnecessary work.

@@ +146,5 @@
> +
> +        ParseState m_parseState;
> +        nsString m_sourceString;
> +        unsigned m_length;
> +        unsigned m_position;

Here, replace unsigned [int] by size_t.

You really want to address arrays with offsets that have the same size as pointers. The integer type that's guaranteed to have pointer size is size_t (for unsigned) or ptrdiff_t (for signed). What's really happening is that on x86-64, arrays can only be addressed using 64bit offsets, so if you provide a 32bit value, you force the compiler to emit an additional instruction to clear the remaining high 32bits in the register used as offset.

See this discussion:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/94a564192d4b4f71
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-09-02 12:13:00 PDT
*** Bug 684312 has been marked as a duplicate of this bug. ***
Comment 10 Daniel Holbert [:dholbert] 2011-09-02 12:47:06 PDT
(In reply to Benoit Jacob [:bjacob] from comment #8)
> > +    nsString cleanSource = StripComments(nsString(source)).result();
> 
> Why don't you declare cleanSource as a const nsString& so that you avoid a
> useless deep copy.

(possibly better: use "const nsAString&")

> > +        nsString result()
> 
> return a const nsString& here, so you don't make a deep-copy of the string
> ;-)

(that applies here, too)
Reference: top entry on https://developer.mozilla.org/en/String_Quick_Reference
Comment 11 Doug Sherk (:drs) (inactive) 2011-09-02 12:57:23 PDT
@dholbert I tried that and got some strange errors, I don't think nsAString is designed to operate in this way.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2011-09-02 13:01:04 PDT
(In reply to Doug Sherk from comment #11)
> @dholbert I tried that and got some strange errors, I don't think nsAString
> is designed to operate in this way.

The crashes we were getting were because the StripComments temporary object was dying too soon; perhaps now that this problem is fixed any other problem you had around that code is fixed too.


@ Daniel:

> (possibly better: use "const nsAString&")

Do you know why nsAString would be better here? This is just a little piece of self-contained code and we know that the string we're dealing with here is really a nsString. Why not just use a nsString reference?
Comment 13 Daniel Holbert [:dholbert] 2011-09-02 13:30:05 PDT
(In reply to Doug Sherk from comment #11)
> @dholbert I tried that and got some strange errors, I don't think nsAString
> is designed to operate in this way.

Perhaps that was from using "nsAString" rather than "nsAString&"? (nsAString is abstract -- you'll get compile errors if you declare an instance of it, IIRC, but references should work fine and be assignable from nsString)

Anyway, doesn't matter much, and I wouldn't waste much time on it if it's causing weird errors.

(In reply to Benoit Jacob [:bjacob] from comment #12)
> Do you know why nsAString would be better here? [...] Why not just use a nsString reference?

More flexibility (ability to swap out underlying string impl) & cross-codebase consistency, I guess?  Regardless, I agree that it's not a big deal in a self-contained piece of code -- I'd just been refreshing my memory on our string guide, and I thought it might be a useful style nit.  I defer to your judgement in this code, of course. :)
Comment 14 Doug Sherk (:drs) (inactive) 2011-09-02 16:11:35 PDT
Created attachment 557978 [details]
Patch v1.1, shader source comment validation fix.

Fixes for previous items, also fixed getShaderSource() to return correct source. This patch is not deployable yet and is mostly here just for reference since more optimization is needed.
Comment 15 Doug Sherk (:drs) (inactive) 2011-09-02 16:28:24 PDT
So I did a little bit of quick research, it was indeed incorrect to be storing the source as UTF-8 as we were doing. gl-getshadersource.html was fixed with the previous patch because of the port to UTF-16.
Comment 16 Doug Sherk (:drs) (inactive) 2011-09-02 19:01:01 PDT
Created attachment 558014 [details] [diff] [review]
Patch v1.2, shader source comment validation fix.

This patch should fix all issues we've been having with comments that have illegal characters, etc. It also fixes gl-getshadersource.html. According to my benchmarks, StripComments.parse() is now ~15% faster (it now takes ~14s to complete comment-stripping-speed-test.html), although in a realistic applications, StripComments.parse() is now called twice. This is because we must validate the source on each call to shaderSource() and compileShader(), so we're now doing comment stripping twice. This was a cost of getting getShaderSource() to work correctly and is also identical to the way WebKit works.

I question whether or not replacing nsString with nsTArray is worth it. The nsTArray implementation is significantly more complex and difficult to understand, and also breaks some encapsulation, while giving little performance benefit. Also, I previously submitted an optimized version of patch v1.0 which equated the buffer string to the input string, then set it back to empty again. This seemed to speed it up almost as much as this implementation, but I didn't debug it to see if what I was doing was even working as intended.

Note that I considered storing copies of both the commented and uncommented shader source, but I assumed this would be bad for mobile devices and it would be preferable to load slower than use more memory.
Comment 17 Doug Sherk (:drs) (inactive) 2011-09-02 19:18:48 PDT
I just tested a version of the comment stripping test with compilation after it and it took 20s to complete, so it's not as bad as I thought. It's a little over twice as slow than no comment stripping at all, and without the fix for gl-getshadersource.html.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 09:21:31 PDT
(In reply to Daniel Holbert [:dholbert] from comment #13)
> I defer to your judgement in this code, of course. :)

Don't do that, I don't know much about our string classes!
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 09:36:44 PDT
(In reply to Doug Sherk from comment #15)
> So I did a little bit of quick research, it was indeed incorrect to be
> storing the source as UTF-8 as we were doing. gl-getshadersource.html was
> fixed with the previous patch because of the port to UTF-16.

Here instead of UTF-8 you meant ASCII. nsCString is ASCII, not unicode. UTF-8 is a flavor of unicode that allow many usual characters to be stored with just 8 bits.
Comment 20 Doug Sherk (:drs) (inactive) 2011-09-06 10:08:23 PDT
(In reply to Benoit Jacob [:bjacob] from comment #19)
> (In reply to Doug Sherk from comment #15)
> > So I did a little bit of quick research, it was indeed incorrect to be
> > storing the source as UTF-8 as we were doing. gl-getshadersource.html was
> > fixed with the previous patch because of the port to UTF-16.
> 
> Here instead of UTF-8 you meant ASCII. nsCString is ASCII, not unicode.
> UTF-8 is a flavor of unicode that allow many usual characters to be stored
> with just 8 bits.

Yep, my bad, thanks for the correction.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 11:27:43 PDT
(In reply to Doug Sherk from comment #16)
> in a realistic
> applications, StripComments.parse() is now called twice. This is because we
> must validate the source on each call to shaderSource() and compileShader(),
> so we're now doing comment stripping twice.

I don't understand why that is. Since shader sources can only be set by shaderSource(), if we already validate them in shaderSource(), why would be also need to validate them in compileShader() ?
Comment 22 Doug Sherk (:drs) (inactive) 2011-09-06 11:43:39 PDT
(In reply to Benoit Jacob [:bjacob] from comment #21)
> (In reply to Doug Sherk from comment #16)
> > in a realistic
> > applications, StripComments.parse() is now called twice. This is because we
> > must validate the source on each call to shaderSource() and compileShader(),
> > so we're now doing comment stripping twice.
> 
> I don't understand why that is. Since shader sources can only be set by
> shaderSource(), if we already validate them in shaderSource(), why would be
> also need to validate them in compileShader() ?

To explain this in more detail, we basically had 3 choices that I could think of in terms of design:

1) When shaderSource() is called, we validate the source, then store the unvalidated source and re-validate it in compileShader().
2) When shaderSource() is called, we validate the source, then store the validated source and use it in compileShader().
3) When shaderSource() is called, we validate the source, then store both the validated and unvalidated source and use the validated source in compileShader().

I chose 1 because 2 breaks getShaderSource() so it's basically out of the question, and 3 uses too much memory. I figured most people complain about memory usage and would be more willing to live with a very slightly slower run (which in most real programs you would never notice) than the extra memory for as long as the context exists. Also, 1 is the way WebKit implements this so maybe they thought of something I didn't.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 11:52:38 PDT
Thanks for the explanation. I agree now with the choice of 1. The memory usage penalty in 3 is not big, but neither is the performance penalty in 1.
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 13:24:18 PDT
Comment on attachment 558014 [details] [diff] [review]
Patch v1.2, shader source comment validation fix.

Review of attachment 558014 [details] [diff] [review]:
-----------------------------------------------------------------

Just need 1 last quick iteration!

::: content/canvas/src/WebGLContextGL.cpp
@@ +3971,5 @@
>                                         SH_WEBGL_SPEC,
>                                         gl->IsGLES2() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT,
>                                         &resources);
>  
> +        StripComments stripComments(shader->Source());

Please add a quick comment explaining why we're constructing a named object here as opposed to just using a StripComments() temporary.

@@ +3972,5 @@
>                                         gl->IsGLES2() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT,
>                                         &resources);
>  
> +        StripComments stripComments(shader->Source());
> +        const nsAString& cleanSource = nsString(&stripComments.result()[0], stripComments.length());

instead of &stripComments.result()[0] do stripComments.result().Elements()

@@ +3979,5 @@
> +
> +        const nsPromiseFlatString& flatSource = PromiseFlatString(cleanSource);
> +
> +        if (!NS_IsAscii(flatSource.get()))
> +            return ErrorInvalidValue("compileShader: non-ascii characters found in source");

Since shaderSource already checks that the source stripped from comments is already in the 7bit ascii range, there is no need for this check here.

(Yes I realize that we had this problem before already)

Just add a comment here explaining that so it's safe to call NS_LossyConvertUTF16toASCII(flatSource)

@@ +4144,5 @@
>      if (!GetConcreteObjectAndGLName("shaderSource: shader", sobj, &shader, &shadername))
>          return NS_OK;
>  
> +    StripComments stripComments(source);
> +    const nsAString& cleanSource = nsString(&stripComments.result()[0], stripComments.length());

Same comments as above.
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2011-09-06 13:48:14 PDT
(In reply to Doug Sherk from comment #16)
> I question whether or not replacing nsString with nsTArray is worth it. The
> nsTArray implementation is significantly more complex and difficult to
> understand, and also breaks some encapsulation, while giving little
> performance benefit.

There, feel free to either go ahead with your nsTArray code since I find it sane, or keep the nsString approach for now if you favor not doing optimizations until they're backed by profiling. Both approaches work with me. I don't find the nsTArray approach too awkward.
Comment 26 Doug Sherk (:drs) (inactive) 2011-09-06 13:51:14 PDT
(In reply to Benoit Jacob [:bjacob] from comment #25)
> (In reply to Doug Sherk from comment #16)
> > I question whether or not replacing nsString with nsTArray is worth it. The
> > nsTArray implementation is significantly more complex and difficult to
> > understand, and also breaks some encapsulation, while giving little
> > performance benefit.
> 
> There, feel free to either go ahead with your nsTArray code since I find it
> sane, or keep the nsString approach for now if you favor not doing
> optimizations until they're backed by profiling. Both approaches work with
> me. I don't find the nsTArray approach too awkward.

I did do some profiling (although not as 100% in-depth as it could have been) and found that the nsTArray approach was faster. Since you prefer the nsTArray implementation, and it's already done, I think it's better to just stick with it.
Comment 27 Doug Sherk (:drs) (inactive) 2011-09-06 14:38:24 PDT
Created attachment 558617 [details] [diff] [review]
Patch v1.2, shader source comment validation fix.

Fixes issues brought up by bjacob.
Comment 28 Doug Sherk (:drs) (inactive) 2011-09-06 18:12:51 PDT
Created attachment 558692 [details] [diff] [review]
Patch v1.3, shader source comment validation fix.

+r carried from patch 1.2 (r=bjacob)

Folded version of the same patch with changes from https://bugzilla.mozilla.org/show_bug.cgi?id=680722
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2011-09-07 14:19:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0bff93ce8
Comment 30 Phil Ringnalda (:philor, back in August) 2011-09-07 15:24:21 PDT
Backed out for Windows burning like https://tbpl.mozilla.org/php/getParsedLog.php?id=6318404
Comment 31 Daniel Holbert [:dholbert] 2011-09-07 15:39:51 PDT
(looks like the burning is in code from another patch in the same push -- bug 681026)
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2011-09-09 15:34:12 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d04d43c81294
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2011-09-09 20:06:01 PDT
http://hg.mozilla.org/mozilla-central/rev/d04d43c81294
Comment 34 neil@parkwaycc.co.uk 2011-09-11 16:31:26 PDT
Comment on attachment 558692 [details] [diff] [review]
Patch v1.3, shader source comment validation fix.

Drive-by string usage review:

>+        // We're storing an actual instance of StripComments because, if we don't, the 
>+        // cleanSource nsAString instance will be destroyed before the reference is
>+        // actually used.
>+        StripComments stripComments(shader->Source());
>+        const nsAString& cleanSource = nsString(stripComments.result().Elements(), stripComments.length());
nsString is a class, so you can write this as
nsString cleanSource(stripComments.result().Elements(), stripComments.length());
However, this makes a copy of the string and null-terminates it, which is inefficient. A more efficient solution is to do
nsDependentSubstring cleanSource(stripComments.result().Elements(),
                                 stripComments.length());
This references the characters in the StripComments member array, but as written, it won't outlive it, so that's OK. It doesn't null-terminate the string, but ValidateGLSLString doesn't need a null-terminated string, so that's also OK.

>+        if (!ValidateGLSLString(cleanSource, "compileShader"))
>+            return NS_OK;
>+
>+        const nsPromiseFlatString& flatSource = PromiseFlatString(cleanSource);
1. NS_LossyConvertUTF16toASCII doesn't require a flat string, so no need to flatten it.
2. Unless you use the nsDependentSubstring, then the string was already flat anyway.

>+        const nsCString& sourceCString = NS_LossyConvertUTF16toASCII(flatSource);
NS_LossyConvertUTF16toASCII is a class, so you can write this as
NS_LossyConvertUTF16toASCII sourceCString(flatSource);
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2011-09-18 22:08:49 PDT
Created attachment 560848 [details] [diff] [review]
address Neil's string comments

> >+        StripComments stripComments(shader->Source());
> >+        const nsAString& cleanSource = nsString(stripComments.result().Elements(), stripComments.length());
> nsString is a class

Right... this bit escaped my attention during review.

>, so you can write this as
> nsString cleanSource(stripComments.result().Elements(),
> stripComments.length());
> However, this makes a copy of the string and null-terminates it, which is
> inefficient. A more efficient solution is to do
> nsDependentSubstring cleanSource(stripComments.result().Elements(),
>                                  stripComments.length());

FWIW, with GCC 4.6 I've run into lots of compilation trouble with this; the #including scheme around nsTDependentSubstring.h makes this hard to figure but even after switching to the Substring functions I couldn't get this form to compile, so I used the form taking 2 character pointers instead.

> >+        const nsPromiseFlatString& flatSource = PromiseFlatString(cleanSource);
> 1. NS_LossyConvertUTF16toASCII doesn't require a flat string, so no need to
> flatten it.

The code immediately below that does require a flat string, so I'm keeping this.

> 2. Unless you use the nsDependentSubstring, then the string was already flat
> anyway.

Well, I do use nsDependentSubstring now :-)

> 
> >+        const nsCString& sourceCString = NS_LossyConvertUTF16toASCII(flatSource);
> NS_LossyConvertUTF16toASCII is a class, so you can write this as
> NS_LossyConvertUTF16toASCII sourceCString(flatSource);

that's what I didn't know. I wish that our function/class naming conventions made it a little more clear what is a class.
Comment 36 neil@parkwaycc.co.uk 2011-09-19 01:18:03 PDT
(In reply to Benoit Jacob from comment #35)
> > A more efficient solution is to do
> > nsDependentSubstring cleanSource(stripComments.result().Elements(),
> >                                  stripComments.length());
> 
> FWIW, with GCC 4.6 I've run into lots of compilation trouble with this; the
> #including scheme around nsTDependentSubstring.h makes this hard to figure
> but even after switching to the Substring functions I couldn't get this form
> to compile, so I used the form taking 2 character pointers instead.
You're right; there are an alarming number of Substring methods but the one I was thinking of only exists in the external string API.

> > >+        const nsPromiseFlatString& flatSource = PromiseFlatString(cleanSource);
> > 1. NS_LossyConvertUTF16toASCII doesn't require a flat string, so no need to
> > flatten it.
> The code immediately below that does require a flat string, so I'm keeping
> this.
Sorry, I don't see where; as far as I can see, flatSource is only used as a parameter to NS_LossyConvertyUTF16toASCII which doesn't need a flat string.
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2011-09-22 16:59:53 PDT
> > > >+        const nsPromiseFlatString& flatSource = PromiseFlatString(cleanSource);
> > > 1. NS_LossyConvertUTF16toASCII doesn't require a flat string, so no need to
> > > flatten it.
> > The code immediately below that does require a flat string, so I'm keeping
> > this.
> Sorry, I don't see where; as far as I can see, flatSource is only used as a
> parameter to NS_LossyConvertyUTF16toASCII which doesn't need a flat string.

We need this NS_LossyConvertyUTF16toASCII to be flat, see just below this code. Do you mean that a NS_LossyConvertyUTF16toASCII is always flat even if the string it was constructed from wasn't?
Comment 38 neil@parkwaycc.co.uk 2011-09-22 17:06:59 PDT
(In reply to Benoit Jacob from comment #37)
> Do you mean that a NS_LossyConvertUTF16toASCII is always flat even if
> the string it was constructed from wasn't?
Indeed, NS_LossyConvertUTF16toASCII is a subclass of nsCAutoString whose constructor calls LossyAppendUTF16toASCII (doesn't need to be LossyCopyUTF16toASCII because the new string is obviously already empty.)

Note You need to log in before you can comment on or make changes to this bug.