If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

javascript: URL results aren't Unicode safe

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(3 attachments)

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...)
Created attachment 219674 [details] [diff] [review]
patch

Asking for darin's review partly to make sure this isn't considered abuse of NS_NewByteInputStream.
Assignee: general → dbaron
Status: NEW → ASSIGNED
Attachment #219674 - Flags: superreview?(brendan)
Attachment #219674 - Flags: review?(darin)
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
> 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
also, looks like a duplicate of bug 278540 to me.
(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
>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
At least mark that old bug as dependent on this one.  I'm ok with forward duping.

/be

Blocks: 278540

Comment 7

12 years ago
Comment on attachment 219674 [details] [diff] [review]
patch

Horray for XXX comments! :-D

r=darin
Attachment #219674 - Flags: review?(darin) → review+
Comment on attachment 219674 [details] [diff] [review]
patch

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

/be
Attachment #219674 - Flags: superreview?(brendan) → superreview+

Comment 9

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

What should it do?  Convert to UTF-8?

Comment 10

12 years ago
I filed bug 335342.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(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
Could this bug have caused bug 335554?
Depends on: 335554
(In reply to comment #13)
> Could this bug have caused bug 335554?

Yes, backing out this patch fixes bug 335554.

Comment 15

12 years ago
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 220793 [details] [diff] [review]
patch to use UTF-8 instead
Attachment #220793 - Flags: superreview?(brendan)
Attachment #220793 - Flags: review?(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
Attachment #220793 - Flags: superreview?(brendan)
Attachment #220793 - Flags: superreview+
Attachment #220793 - Flags: review?(brendan)
Attachment #220793 - Flags: review+
Checked UTF-8 version into trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #219674 - Flags: approval-branch-1.8.1?(brendan)
Attachment #220793 - Flags: approval-branch-1.8.1?(brendan)
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).

Updated

12 years ago
Attachment #219674 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+

Updated

12 years ago
Attachment #219674 - Flags: approval-branch-1.8.1+

Updated

12 years ago
Attachment #220793 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Created attachment 220831 [details] [diff] [review]
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).

Updated

12 years ago
Attachment #219674 - Flags: approval-branch-1.8.1+
Fix checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
*** Bug 278540 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Depends on: 355358
You need to log in before you can comment on or make changes to this bug.