Unload Piex NaCl module when idling.

Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening a RAW file in
     Files app then waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2714413004
Cr-Commit-Position: refs/heads/master@{#453897}
Committed: https://chromium.googlesource.com/chromium/src/+/8f4fd7677a68e465ba2db9c6c1eb969d0d9c8c73

[mtomasz, 2017-02-28 10:22:05]

Description was changed from

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening RAW, then closing
     Files app and waiting a few seconds for the image loader to go away.
BUG=517741
==========

to

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening RAW, then closing
     Files app and waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-02-28 10:32:31]

Description was changed from

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening RAW, then closing
     Files app and waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening RAW, then closing
     Files app and waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-02-28 10:32:31]

mtomasz@chromium.org changed reviewers:
+ yamaguchi@chromium.org

[mtomasz, 2017-02-28 10:33:08]

Description was changed from

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening RAW, then closing
     Files app and waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening a RAW file in
     Files app then waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-02-28 10:35:05]

@yamaguchi: PTAL. Sadly I don't see an easy way to add tests for that, as the
Piex NaCl module is not available in Chromium. I could add tests by creating
some mock NaCl module or even a JS wrapper for a NaCl module, but not sure if
it's feasible. What do you think?

[mtomasz, 2017-02-28 10:35:11]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 10:35:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 11:03:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 11:03:02]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-02-28 13:57:38]

lgtm

https://codereview.chromium.org/2714413004/diff/20001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader.js (right):

https://codereview.chromium.org/2714413004/diff/20001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:110: * Idling time before the NaCl
module is unloaded. This let's the image loader
nit: let's --> lets

[mtomasz, 2017-03-01 03:56:06]

@yamaguchi: PTAL one more time as in the end I added tests. Thanks.

https://codereview.chromium.org/2714413004/diff/20001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader.js (right):

https://codereview.chromium.org/2714413004/diff/20001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:110: * Idling time before the NaCl
module is unloaded. This let's the image loader
On 2017/02/28 13:57:37, yamaguchi wrote:
> nit: let's --> lets

Done.

[mtomasz, 2017-03-01 03:59:32]

mtomasz@chromium.org changed reviewers:
+ hirono@chromium.org

[mtomasz, 2017-03-01 03:59:48]

@hirono: FYI.

[hirono, 2017-03-01 04:03:18]

lgtm. Thank you for fixing this!

[mtomasz, 2017-03-01 04:30:30]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 04:30:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 04:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 04:58:26]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-03-01 07:28:47]

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:58: * @param {function()=}
opt_createModule Creates a NaCl module.
How about adding description how it behaves when this is null or omitted?
I also prefer having the name like "opt_customModuleCreator" or
"opt_moduleCreatorFunction", but opt_createModule would be OK as long as it's
consistent with other ones.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:59: * @param {function(!Element)=}
opt_destroyModule Destroys a NaCl module.
ditto.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:290:
clearTimeout(this.unloadTimer_);
Would it be safer to clear timeout first, because timer may trigger while the
handler is executed and thus call the handler twice?
(Though I think that would not happen with the current usage.)

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader_unittest.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:18: }
; missing

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:20:
MockModule.prototype.decorate = function() {
Fix indent.

[yamaguchi, 2017-03-01 08:03:59]

Added a couple of comments with questions on the test execution.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader_unittest.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:56:
loader.simulateIdleTimeoutPassedForTests();
Does this mean we want to trigger the idle timeout every time loading an image,
before sending back the message in the test?

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:77: // The NaCl module is
not unloaded, as the next request came
IIUC, this will be called after the Promise by load() is resolved, which means
it's after postMessage is called.
I guessed that, if the function set by setTimeout() in
MockModule.prototype.postMessage were executed first (as it's scheduled with
timeout=0) like a race condition, it'll invoke onBeforeMessageCallback_ and will
unload the module before reaching here.

[mtomasz, 2017-03-01 08:30:51]

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:58: * @param {function()=}
opt_createModule Creates a NaCl module.
On 2017/03/01 07:28:46, yamaguchi wrote:
> How about adding description how it behaves when this is null or omitted?

Done.

> I also prefer having the name like "opt_customModuleCreator" or
> "opt_moduleCreatorFunction", but opt_createModule would be OK as long as it's
> consistent with other ones.

I tried many names and realized that functions usually contain a verb.
"Function" or "Callback" suffix are common, but not sure if they add any value
here?

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:59: * @param {function(!Element)=}
opt_destroyModule Destroys a NaCl module.
On 2017/03/01 07:28:46, yamaguchi wrote:
> ditto.

Added in class description. Let me know if you prefer it to be in @param.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader.js:290:
clearTimeout(this.unloadTimer_);
On 2017/03/01 07:28:47, yamaguchi wrote:
> Would it be safer to clear timeout first, because timer may trigger while the
> handler is executed and thus call the handler twice?
> (Though I think that would not happen with the current usage.)

Done.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader_unittest.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:18: }
On 2017/03/01 07:28:47, yamaguchi wrote:
> ; missing

Done.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:20:
MockModule.prototype.decorate = function() {
On 2017/03/01 07:28:47, yamaguchi wrote:
> Fix indent.

Done.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:56:
loader.simulateIdleTimeoutPassedForTests();
On 2017/03/01 08:03:59, yamaguchi wrote:
> Does this mean we want to trigger the idle timeout every time loading an
image,
> before sending back the message in the test?

The idea behind this call was to check in test below that even if NaCl module
was very slow, we wouldn't unload the NaCl module. That would cause an error in
loading the image.

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:77: // The NaCl module is
not unloaded, as the next request came
On 2017/03/01 08:03:59, yamaguchi wrote:
> IIUC, this will be called after the Promise by load() is resolved, which means
> it's after postMessage is called.

Right.

> I guessed that, if the function set by setTimeout() in
> MockModule.prototype.postMessage were executed first (as it's scheduled with
> timeout=0) like a race condition, it'll invoke onBeforeMessageCallback_ and
will
> unload the module before reaching here.

Indeed it will be called in this sequence. This test verifies that if timeout
happens before completing the promise, then the NaCl module won't be unloaded.
And it won't be unloaded, as PiexLoader.unloadTimer_ will be 0. The timer is
cleared when a new request is added to PiexLoader.requests_.

The critical line is this one:

PiexLoader.prototype.simulateIdleTimeoutPassedForTests = function() {
-->  if (this.unloadTimer_) {
    clearTimeout(this.unloadTimer_);
    this.onIdleTimeout_();
  }
};

I agree that the logic is not very obvious here. I added some comments to the
code.

[yamaguchi, 2017-03-01 09:04:12]

lgtm

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
File ui/file_manager/image_loader/piex_loader_unittest.js (right):

https://codereview.chromium.org/2714413004/diff/40001/ui/file_manager/image_l...
ui/file_manager/image_loader/piex_loader_unittest.js:77: // The NaCl module is
not unloaded, as the next request came
On 2017/03/01 08:30:51, mtomasz wrote:
> On 2017/03/01 08:03:59, yamaguchi wrote:
> > IIUC, this will be called after the Promise by load() is resolved, which
means
> > it's after postMessage is called.
> 
> Right.
> 
> > I guessed that, if the function set by setTimeout() in
> > MockModule.prototype.postMessage were executed first (as it's scheduled with
> > timeout=0) like a race condition, it'll invoke onBeforeMessageCallback_ and
> will
> > unload the module before reaching here.
> 
> Indeed it will be called in this sequence. This test verifies that if timeout
> happens before completing the promise, then the NaCl module won't be unloaded.
> And it won't be unloaded, as PiexLoader.unloadTimer_ will be 0. The timer is
> cleared when a new request is added to PiexLoader.requests_.
> 
> The critical line is this one:
> 
> PiexLoader.prototype.simulateIdleTimeoutPassedForTests = function() {
> -->  if (this.unloadTimer_) {
>     clearTimeout(this.unloadTimer_);
>     this.onIdleTimeout_();
>   }
> };
> 
> I agree that the logic is not very obvious here. I added some comments to the
> code.

Thanks for the explanation. I could understand it now.

[mtomasz, 2017-03-01 09:54:52]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2017-03-01 09:54:53]

The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org
Link to the patchset: https://codereview.chromium.org/2714413004/#ps60001
(title: "Fix issues + add comments.")

[commit-bot: I haz the power, 2017-03-01 09:55:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 10:30:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488362092967490,
"parent_rev": "6f0e5af5c878bb2e9857fa2247b20b05475cee80", "commit_rev":
"8f4fd7677a68e465ba2db9c6c1eb969d0d9c8c73"}

[commit-bot: I haz the power, 2017-03-01 10:31:18]

Description was changed from

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening a RAW file in
     Files app then waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Unload Piex NaCl module when idling.

Chrome no longer unloads extensions which have an active NaCl module.
This CL fixes this problem by unloading the NaCl module after 3 seconds
since the last completed request to open a RAW file.

TEST=Tested manually on Chrome with Piex by opening a RAW file in
     Files app then waiting a few seconds for the image loader to go away.
BUG=517741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2714413004
Cr-Commit-Position: refs/heads/master@{#453897}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f4fd7677a68e465ba2db9c6c1eb...
==========

[commit-bot: I haz the power, 2017-03-01 10:31:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8f4fd7677a68e465ba2db9c6c1eb...