335298 - javascript: URL results aren't Unicode safe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

javascript: URLs don't properly handle Unicode data returned from the javascript expression (see above URL for an example).  The main problem is this chunk in nsJSThunk::EvaluateString:

    else {
        // NS_NewStringInputStream calls ToNewCString
        // XXXbe this should not decimate! pass back UCS-2 to necko
        rv = NS_NewStringInputStream(getter_AddRefs(mInnerStream), result);
    }

but there may also be a need to set a character encoding on a channel somewhere.  (And I'll note that I recently wanted to make a string input stream from a string in javascript that may have had wide characters, but couldn't...)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Asking for darin's review partly to make sure this isn't considered abuse of NS_NewByteInputStream.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

> And I'll note that I recently wanted to make a string input stream
> from a string in javascript that may have had wide characters, but couldn't...

http://lxr.mozilla.org/seamonkey/source/intl/uconv/idl/nsIScriptableUConv.idl#91

as for the patch:
+                                  contentType, &contentCharset);

you shouldn't need to use a pointer here

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

also, looks like a duplicate of bug 278540 to me.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> also, looks like a duplicate of bug 278540 to me.

How could it be a duplicate of a bug whose summary doesn't mention javascript: URLs?

(In reply to comment #2)
> as for the patch:
> +                                  contentType, &contentCharset);
> 
> you shouldn't need to use a pointer here

Yes I should:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/public/nsNetUtil.h&rev=1.98&mark=296#291

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

>How could it be a duplicate of a bug whose summary doesn't mention javascript:
>URLs?

well, bug 278540 comment 5 sounds like it refers to the same comment that you're removing.

> Yes I should:

No you shouldn't :-)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/public/nsNetUtil.h&rev=1.98&mark=335#330

Comment 6

[Brendan Eich [:brendan]]

At least mark that old bug as dependent on this one.  I'm ok with forward duping.

/be

Comment 7

[Darin Fisher]

Comment on attachment 219674 [details] [diff] [review]
patch

Horray for XXX comments! :-D

r=darin

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 219674 [details] [diff] [review]
patch

Howzabout a followup bug for a typesafe NS_NewStringInputStream API variant for this case?

/be

Comment 9

[Darin Fisher]

> Howzabout a followup bug for a typesafe NS_NewStringInputStream API variant for
> this case?

What should it do?  Convert to UTF-8?

Comment 10

[Darin Fisher]

I filed bug 335342.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #9)
> > Howzabout a followup bug for a typesafe NS_NewStringInputStream API variant for
> > this case?
> 
> What should it do?  Convert to UTF-8?

No, I meant there ought to be a type-safe way to pass a string in native UTF-16 byte order from producer to consumer, all in memory.  dbaron had to cast to byte pointer.  I'm looking for a typed interface that avoids that.

/be

Comment 13

[Steve England [:stevee]]

Could this bug have caused bug 335554?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Could this bug have caused bug 335554?

Yes, backing out this patch fixes bug 335554.

Comment 15

[james davis]

does this need to be reopened and re-evaluated to address
what it did to cause bug https://bugzilla.mozilla.org/show_bug.cgi?id=335554

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, I claim that's most likely the shockwave plugin depending on an unfrozen API, but I could redo this to use UTF-8 instead and see if that helps.  That's probably easier that actually trying to figure out what the plugin is doing.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to use UTF-8 instead

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 220793 [details] [diff] [review]
patch to use UTF-8 instead

It would be good to get a little of jst's and an Adobe Flash person's time to consider what caused the regression.  I'll mail this bug around.

/be

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked UTF-8 version into trunk.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The UTF-8 version did fix the regression, per bug 335554 comment 16, so the Shockwave plugin is looking at the raw data resulting from a javascript: URL without checking its encoding.  That said, the UTF-8 patch isn't really any worse, and is probably a better approach in general for others doing this to copy (see bug 336513, where I suggested APIs to make this easier).

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: combined patch for 1.8 branch

This is the combined patches for the 1.8 branch (also with the same workaround I used in nsIconChannel for lack of NS_ASSIGNMENT_ADOPT on that branch).

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 23

[Boris Zbarsky [:bzbarsky]]

*** Bug 278540 has been marked as a duplicate of this bug. ***