Last Comment Bug 898221 - Wasted work in punycode_decode()
: Wasted work in punycode_decode()
: perf
Product: Core
Classification: Components
Component: Networking: DNS (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla25
Assigned To: pchang9
: Patrick McManus [:mcmanus]
Depends on: 935499
  Show dependency treegraph
Reported: 2013-07-25 18:07 PDT by pchang9
Modified: 2013-11-13 10:58 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Suggested patch (908 bytes, patch)
2013-07-25 18:07 PDT, pchang9
no flags Details | Diff | Splinter Review
buildable patch (1.06 KB, patch)
2013-07-29 13:06 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review

Description User image pchang9 2013-07-25 18:07:09 PDT
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.
Comment 1 User image Patrick McManus [:mcmanus] 2013-07-29 04:43:01 PDT
pchang9 - thanks for the contribution.

Nick, can you shepherd this?
Comment 2 User image Nicholas Hurley [:nwgh][:hurley] 2013-07-29 10:47:10 PDT
(In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #1)
> pchang9 - thanks for the contribution.
> Nick, can you shepherd this?


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 3 User image Nicholas Hurley [:nwgh][:hurley] 2013-07-29 11:07:14 PDT
Comment on attachment 781379 [details] [diff] [review]
Suggested patch

Adding myself as reviewer per Patrick's email
Comment 4 User image Nicholas Hurley [:nwgh][:hurley] 2013-07-29 11:31:54 PDT
Running through try right now:
Comment 5 User image Nicholas Hurley [:nwgh][:hurley] 2013-07-29 13:06:09 PDT
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 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.
Comment 6 User image Nicholas Hurley [:nwgh][:hurley] 2013-07-30 13:04:26 PDT
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!
Comment 7 User image Ryan VanderMeulen [:RyanVM] 2013-07-30 18:40:08 PDT

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