window.atob and window.btoa are not implemented. I would prefer them as parts of the jsengine, but Brendan E. prefers them to reside inside the host implementation.
Confirming in Build 2002013003, win98 ->OS All
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Status: NEW → ASSIGNED
Priority: -- → P2
Hardware: PC → All
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.9
Bz, Brendan, reviews?
Comment on attachment 67553 [details] [diff] [review] Proposed fix. You're leaking |base64| and |ascii| in both those functions on success. You want to free them. Also, I don't think you need the resultLen stuff. I assume the data channel just does this because it's faster than an implicit strlen (which is what the nsDependentCString you use in Atob does). I'd just use the dependent CString for both. Finally, is there something somewhere that explains exactly what atob and btoa should be doing? Georg?
Attachment #67553 - Flags: needs-work+
http://lxr.mozilla.org/classic/source/lib/libmocha/lm_win.c#2741 and below provide source for the Classic atob and btoa. /be
Er, duh, silly leaks! I do need resultLen since there can be embedded nulls encoded in the base64 encoded string that comes into btoa(), strlen() would give the wrong answer in such a case. The implicit strlen() in atob() is fine since the string is base64 encoded at that point and will not contain nulls even if the ascii string that came in to atob() contained embedded nulls.
Brendan, yes, but that code uses ATOB_AsciiToData() (n' friend), which according to lxr always return null (?), see http://lxr.mozilla.org/classic/ident?i=ATOB_AsciiToData
OK. I was just asking because btoa is apparently an algorithm in its own right distinct from uuencode and base64 (except in Navigator, apparently)... jst, wouldn't it make more sense to change PL_Base64Decode to return the length (as ATOB_AsciiToData does)? That way all its callers won't have to copy the length-calculation code....
jst, sorry -- I should have followed that link. It dangles because ATOB_AsciiData was for some silly reason obfuscated or stubbed out along with a bunch of classic crypto code. I found descendents of the old code at and above http://lxr.mozilla.org/mozilla/source/security/nss/lib/util/nssb64d.c#813 -- I trust these preserve all the semantics. Cc'ing relyea. /be
Brendan, do you for some reason think that the nspr base64 code wouldn't do the right thing here? Is there some reason for using the nss ones over the nspr ones? If so, I'd need to link against the nss code unless those methods are exposed through some XPCOM interfaces, which is something I'd rather not do unless there's no better way out.
bz, yes, that would make a lot of sense, but I won't start changing nspr API's just becuase of this...
jst, I have no opinion about the NSPR vs. NSS implementations of btoa and atob -- in fact I wonder why we have two in closely related modules. I was struck by the fact that classic used precursors of the NSS routines. Cc'ing wtc. /be
I am not familiar with the nspr base64 code and the nss atob and btoa code. Sorry.
I think the NSS base 64 code is streaming while the NSPR base 64 code is one shot. Ideally NSPR's version should support streaming and the NSS version should disappear. This has been a long standing issue. At one point in the Navigator 3.0 developement there was as many as 5 different impementations of base64. Anyway this case I suggest using the NSPR versions if at all possible. BTW, as for linking against NSS directly, while I don't recommend it (you can confuse PSM if you tweak the wrong NSS function), when we land nss 3.4, it would be possible because nss is now compiled in separate dll's. Again, for almost any other function this would not work (you would interfere PSM's usages), and you would require BUILD_PSM2 to be always set to build mozilla if you tried this. bob
Created attachment 67847 [details] [diff] [review] Er, that wasn't right, but this should be...
Comment on attachment 67847 [details] [diff] [review] Er, that wasn't right, but this should be... r=bzbarsky
Attachment #67847 - Flags: review+
Created attachment 68062 [details] [diff] [review] More understandable patch, same as above, just renaming things a bit... This one's ready to go...
Attachment #67847 - Attachment is obsolete: true
Comment on attachment 68062 [details] [diff] [review] More understandable patch, same as above, just renaming things a bit... r=bzbarsky
Attachment #68062 - Flags: review+
Comment on attachment 68062 [details] [diff] [review] More understandable patch, same as above, just renaming things a bit... sr=vidur
Attachment #68062 - Flags: superreview+
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.