Max Schaefer Reporter
	Description • 10 years ago

Line 124 (https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L124) has the following test:

  if (!Module['load'] == 'undefined' && Module['read']) {

Note that "!Module['load']" is a Boolean, so it can never be equal to the string "undefined".

	Mike Lee [:mlee]
	Comment 1 • 10 years ago

Thanks Max!

Luke,

Looks like this problem was introduced in your changes for Bug 900901 "Replace the pinyin IME engine with the emscripten'ed one".

This error will cause Module.load() to be undefined rather than implemented using the Module.read() fallback implementation in non-Node[1] and non-WebWorker[2] environments:

  if (!Module['load'] == 'undefined' && Module['read']) {
    Module['load'] = function(f) {
      globalEval(Module['read'](f));
    };
  }

Can you fix this?

Thanks!
Mike


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L67
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L115

	Luke Chang [:lchang] (inactive)
	Comment 2 • 10 years ago

Max, Thanks for identifying.

I currently have some bugs need to be handled first so I'll take a look in a few more days. Prior to it, if anyone would like to take this bug, please feel free to do that. :)

	Achal Assignee
	Comment 3 • 10 years ago

Hello,

As Max mentioned that it is just a bad comparison which never evaluates to true, I have corrected it, please review the PR. I don't know to test it, hoping it passes tbpl.

	Luke Chang [:lchang] (inactive)
	Comment 4 • 10 years ago

Hi Achal,

Thanks for your help and assign to you since you've had a patch. The patch is good to me. Let's call for keyboard owner's review.

	Luke Chang [:lchang] (inactive)
	Comment 5 • 10 years ago

Comment on attachment 8484442 [details]
PR to correct the first condition.

Hi Rudy,

Could you help to review Achal's patch? Thanks.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 6 • 10 years ago

Comment on attachment 8484442 [details]
PR to correct the first condition.

r=me.
Thanks for patching this up.

Luke,

May I know if this part of code is generated by emscripten or manually written?
If this is generated by emscripten, then maybe the upstream code might need to be fixed as well.

	Luke Chang [:lchang] (inactive)
	Comment 7 • 10 years ago

Hi Rudy,

Yes, this part is generated by emscripten. Agree with you that we may need to notify them about this issue.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 8 • 10 years ago

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/e84af5486c3b513a39884e6dac42b67f6a2a855b

--
Achal, Luke,
Thanks for working on this.

--
[CI status]
The Gij failures happened in different test cases within 3 run, so I don't think they're relevant to this change.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 9 • 10 years ago

Just out of curiosity, Max, could you share with us how you find this issue?
You're studying this part of code or try to make it work on plain web?

	Max Schaefer Reporter
	Comment 10 • 10 years ago

I found it using a static analysis tool.

	Achal Assignee
	Comment 11 • 10 years ago

Hello Rudy, thanks for reviewing and merging the patch. :)

	Mike Lee [:mlee]
	Comment 12 • 10 years ago

Hi Alon,

Are you the right contact to notify re: possible Emscripten issues? If not can you pass this issue on to whomever that is? See comment 0, comment 6 and comment 7 for details.

Thanks,
Mike

(In reply to Rudy Lu [:rudyl] from comment #6)
>
> ...
> 
> Luke,
> 
> May I know if this part of code is generated by emscripten or manually
> written?
> If this is generated by emscripten, then maybe the upstream code might need
> to be fixed as well.

(In reply to Luke Chang [:lchang] from comment #7)
> Hi Rudy,
> 
> Yes, this part is generated by emscripten. Agree with you that we may need
> to notify them about this issue.

	Alon Zakai (:azakai)
	Comment 13 • 10 years ago

Fixed in upstream, thanks!