Last Comment Bug 1116227 - String shims are defined incorrectly
: String shims are defined incorrectly
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Provider: GData (show other bugs)
: unspecified
: Other Other
-- normal (vote)
: 4.0.0.1
Assigned To: Philipp Kewisch [:Fallen]
:
:
Mentors:
Depends on:
Blocks: 1117541
  Show dependency treegraph
 
Reported: 2014-12-29 11:53 PST by Jonathan Cutting
Modified: 2015-01-27 16:14 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Testing XPI for this bug (3.67 KB, application/zip)
2015-01-05 13:11 PST, Jonathan Cutting
no flags Details
Fix - v1 (2.65 KB, patch)
2015-01-20 04:11 PST, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v1 (2.71 KB, patch)
2015-01-21 04:18 PST, Philipp Kewisch [:Fallen]
matthew.mecca: review+
Details | Diff | Splinter Review

Description User image Jonathan Cutting 2014-12-29 11:53:33 PST
User Agent: Mozilla/5.0 (X11; CrOS armv7l 6310.68.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.96 Safari/537.36

Steps to reproduce:

Installed Provider for Google Calendar and tried to run the code on this page https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICryptoHash as detailed below:

var str = "hello world";
var converter =
  Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].
    createInstance(Components.interfaces.nsIScriptableUnicodeConverter);

// we use UTF-8 here, you can choose other encodings.
converter.charset = "UTF-8";
// result is an out parameter,
// result.value will contain the array length
var result = {};
// data is an array of bytes
var data = converter.convertToByteArray(str, result);
var ch = Components.classes["@mozilla.org/security/hash;1"]
                   .createInstance(Components.interfaces.nsICryptoHash);
ch.init(ch.MD5);
ch.update(data, data.length);
var hash = ch.finish(false);

// return the two-digit hexadecimal code for a byte
function toHexString(charCode)
{
  return ("0" + charCode.toString(16)).slice(-2);
}

// convert the binary hash data to a hex string.
var s = [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
// s now contains your hash in hex: should be
// 5eb63bbbe01eeed093cb22bb8f5acdc3


Actual results:

The hash variable returned from ch.finish had a length property of 16, but the lower code section containing for (i in hash) looped 17 times, meaning the eventual md5sum was 34 chars in length not 32


Expected results:

The loop should have iterated 16 times returning a 32 char md5sum. Uninstalling or downgrading the Provider for Google Calendar extension fixes the problem.
Comment 1 User image Benjamin Smedberg [:bsmedberg] 2015-01-05 10:53:56 PST
Does the provider for google calendar replace nsICryptoHash with a new implementation?

Have you debugged this at all to see why this is happening? Without debugging, it's not clear that this is a Mozilla bug instead of just a bug in the extension somehow.
Comment 2 User image Jonathan Cutting 2015-01-05 13:11:24 PST
Created attachment 8544191 [details]
Testing XPI for this bug
Comment 3 User image Jonathan Cutting 2015-01-05 13:12:43 PST
I've created a small xpi that just runs the test md5 code (attached to this bug). If you run it with the Provider installed, the md5sum generated is 34 chars long. Remove the Provider extension and the md5sum is the expected length.
Comment 4 User image Jonathan Cutting 2015-01-05 13:23:31 PST
Sorry forgot to answer your questions. The author of the Provider extension responded to a forum post here: https://groups.google.com/forum/#!topic/provider-for-google-calendar/Rm2F0UtV3xU - it seems the extension does not make use of nsiCryptoHash.

Version 1.0.2 of the Provider extension does not provoke the bug. I ran a recursive diff on the versions but I've not come across anything that would obviously cause this behaviour.
Comment 5 User image Philipp Kewisch [:Fallen] 2015-01-05 14:38:41 PST
(I'm the author by the way)

I do not make any changes to nsICryptoHash in the Provider extension, nor do I even use it. Between 1.0.2 and 1.0.3 I made a lot of changes to support older gecko versions, mostly shimming JS modules. I don't do any xpcom registrations though. If you want to take a brief look at the shims, they are all here: http://mxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/shim/
Comment 6 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2015-01-06 09:48:55 PST
The last property is 'includes'. Commenting http://mxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/shim/Loader.jsm#70 out fixes the issue.

Run this with remote debugging to see it in the console:
for (var i in "abcdefghijklmnop") console.log("character: " + i)
Comment 7 User image Benjamin Smedberg [:bsmedberg] 2015-01-06 11:14:05 PST
Great thanks. This is a bug in the extension, which should be using .defineProperty to create a non-enumerable expando on String.
Comment 8 User image Philipp Kewisch [:Fallen] 2015-01-06 15:01:06 PST
Thanks for the analysis! Moving this bug back to calendar land to fix it.
Comment 9 User image Philipp Kewisch [:Fallen] 2015-01-20 04:11:29 PST
Created attachment 8551744 [details] [diff] [review]
Fix - v1

This should take care.
Comment 10 User image Philipp Kewisch [:Fallen] 2015-01-21 04:18:29 PST
Created attachment 8552417 [details] [diff] [review]
Fix - v1
Comment 11 User image Matthew Mecca [:mmecca] 2015-01-26 21:32:23 PST
Comment on attachment 8552417 [details] [diff] [review]
Fix - v1

Looks good, r=mmecca
Comment 12 User image Philipp Kewisch [:Fallen] 2015-01-27 16:14:51 PST
Pushed to comm-central changeset c785d6f3f52a

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