Closed Bug 673296 Opened 9 years ago Closed 8 years ago

Origin handling in EventSource is bogus

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: wfernandom2004, Assigned: wfernandom2004)

References

Details

Attachments

(1 file, 2 obsolete files)

Blocks: 664179
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #547568 - Flags: review?(jonas)
Attachment #547568 - Flags: review?(Olli.Pettay)
Why is that getting the origin from the URI and not from the principal?
Comment on attachment 547568 [details] [diff] [review]
v1

r=me if you address bz' comment.
Attachment #547568 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 547568 [details] [diff] [review]
v1

Hmm.. mUTF16Origin is a somewhat misleading name. GetASCIIOrigin returns the puny-code encoded origin. I would have expected mUTF16Origin to contain a unicode encoded origin based on the explicit 'utf16' in the name.

I'd also have guessed that the event should contain the unicode variant.

Is there a reason you explicitly ask for the punycode coded variant?
Sicking, did you look at bug 67068?
Comment on attachment 547568 [details] [diff] [review]
v1

Actually, no, this is not right
Message events don't need ascii representation of the origin.
Attachment #547568 - Flags: review+ → review-
"the origin attribute must be set to the Unicode serialization of the origin of the event stream's URL"
But we may need to do something to the origin,
see https://bugzilla.mozilla.org/show_bug.cgi?id=670687#c5
nsContentUtils::GetUTFOrigin should do the right thing.


(GetXXXOrigin really should not be in nsContentUtils)
> Why is that getting the origin from the URI and not from the principal?
Because "the origin attribute must be set to the Unicode serialization of the origin of the event stream's URL". 

I don't know if I have misunderstood that, but I think per spec it is correct. WebSockets uses the origin from the principal, which is get from the entry script. It seems strange to me, but the specs define these behaviors...

> nsContentUtils::GetUTFOrigin should do the right thing.
ok
> mUTF16Origin is a somewhat misleading name.
So I will let mOrigin
Attached patch v2 (obsolete) — Splinter Review
Attachment #547568 - Attachment is obsolete: true
Attachment #547568 - Flags: review?(jonas)
Attachment #549658 - Flags: review?(jonas)
Attachment #549658 - Flags: review?(Olli.Pettay)
Comment on attachment 549658 [details] [diff] [review]
v2

Could we get some tests for this.
Attachment #549658 - Flags: review?(Olli.Pettay) → review+
> Could we get some tests for this.
In bug 664179 we have already some tests.
Attached patch v3Splinter Review
updated for bug 664179
Attachment #549658 - Attachment is obsolete: true
Attachment #570446 - Flags: review?(jonas)
Comment on attachment 570446 [details] [diff] [review]
v3

You don't actually need to re-request review when just updating to tip.
Attachment #570446 - Flags: review?(jonas) → review+
> You don't actually need to re-request review when just updating to tip.
Ok, sorry. I didn't know that.
Checked in to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab7b95bfcb5

Thanks for the quick turnaround and for the patch in general!! Please do poke someone if you see bugs lingering like this, that should never happen.
https://hg.mozilla.org/mozilla-central/rev/3ab7b95bfcb5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Backed out in https://hg.mozilla.org/mozilla-central/rev/abdbf0646a21 - see bug 664179 comment 85
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Fixed on bug 664179. See https://bugzilla.mozilla.org/show_bug.cgi?id=664179#c97
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.