Wasted work in punycode_decode()

RESOLVED FIXED in mozilla25

Status

()

Core
Networking: DNS
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pchang9, Assigned: pchang9)

Tracking

({perf})

Trunk
mozilla25
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 781379 [details] [diff] [review]
Suggested patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130116073211

Steps to reproduce:

The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it.

In method punycode_decode() in netwerk/dns/punycode.c, the loop in line 218 keeps overriding "b" with "j" when "delim(input[j])" is true. Therefore, only the last written value is visible out of the loop and all the other writes and iterations are not necessary. The patch iterates from the end of "j" and breaks the first time when "b" is set.
(Assignee)

Updated

4 years ago
Keywords: perf
OS: Windows 7 → All
Version: 18 Branch → Trunk

Updated

4 years ago
Assignee: nobody → pchang9
Component: Untriaged → Networking: DNS
Product: Firefox → Core
Whiteboard: patch

Updated

4 years ago
Attachment #781379 - Attachment is patch: true
pchang9 - thanks for the contribution.

Nick, can you shepherd this?
(In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #1)
> pchang9 - thanks for the contribution.
> 
> Nick, can you shepherd this?

Absolutely.

pchang9, thanks! I'll take a look at the patch, and run it through our test suite just to be sure. Assuming everything checks out (I don't see why it wouldn't), we'll get this landed in Firefox.
Comment on attachment 781379 [details] [diff] [review]
Suggested patch

Adding myself as reviewer per Patrick's email
Attachment #781379 - Flags: review?(hurley)
Running through try right now: https://tbpl.mozilla.org/?tree=Try&rev=c11ecf46a7b4
Created attachment 782725 [details] [diff] [review]
buildable patch

pchang9 - the original patch didn't compile due to comparing an unsigned value with a signed value (that is one of the many warnings that cause compile errors in mozilla code). I took the liberty of fixing that in the patch, and have pushed the new version to try https://tbpl.mozilla.org/?tree=Try&rev=92bed275dfae The updated version has been uploaded for you to take a look at.

Once this version passes try, we can go ahead and get this patch landed in mozilla-central.
Attachment #781379 - Attachment is obsolete: true
Attachment #781379 - Flags: review?(hurley)
Attachment #782725 - Flags: review+
pchang9 - as you can see if you look at the link in comment 5, the patch has passed our tests, and I have landed it on mozilla-inbound for you (the link is below). This should be merged to mozilla-central in a day or so, depending on the sheriff schedule.

Thanks again for the patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/515d75612e53
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/515d75612e53
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 935499
Blocks: 935499
No longer depends on: 935499
No longer blocks: 935499
Depends on: 935499
You need to log in before you can comment on or make changes to this bug.