1338138 - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-blocked-content.js | test-blocked-content.js::test_paste_file_urls

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

First seen: Thu Feb 9, 2017, 7:47:00
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=450c53f428f5a590658ab880b8cf28d590c59ae9

Last good: M-C: 5e17f9181c6c
First bad: M-C: f505911eb333

Range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e17f9181c6c&tochange=f505911eb333

Error according to log:
INFO -  SUMMARY-PASS | test-blocked-content.js::setupModule
INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-blocked-content.js | test-blocked-content.js::test_paste_file_urls
INFO -    EXCEPTION: Timeout waiting for pasted tmp image to be loaded ok
INFO -      at: utils.js line 447
INFO -         TimeoutError utils.js:447 13
INFO -         waitFor utils.js:485 11
INFO -         MozMillController.prototype.waitFor controller.js:687 3
INFO -         test_paste_file_urls test-blocked-content.js:100 3
INFO -         Runner.prototype.wrapper frame.js:585 9
INFO -         Runner.prototype._runTestModule frame.js:655 9
INFO -         Runner.prototype.runTestModule frame.js:701 3
INFO -         Runner.prototype.runTestDirectory frame.js:525 7
INFO -         runTestDirectory frame.js:707 3
INFO -         Bridge.prototype._execFunction server.js:179 10
INFO -         Bridge.prototype.execFunction server.js:183 16
INFO -         Session.prototype.receive server.js:283 3
INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 1

[Jorg K (CEST = GMT+2)]

Magnus, your new test is already failing ;-(

It's really funny since I copied that section into a test I wrote, well, patch-worked together, and that doesn't fail, but the original does.

Comment 2

[Jorg K (CEST = GMT+2)]

Console says:
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 2307: TypeError: file.lastModifiedDate is undefined.

So this is caused by
Bug 1335536 - File.createFromNsIFile and File.createFromFileName should be async 

Basically we need to replace the:
let file = File.createFromNsIFile(nsFile);
with something from https://hg.mozilla.org/mozilla-central/rev/b6a45e1bab4c

Let me see whether I find a good example for a replacement.

Comment 3

[alta88]

Attachment: date.patch

since i don't want anymore backouts of my patches ;) here's the fix. maybe it was Bug 1335536, or maybe deprecated lastModifiedDate finally was removed or something.  test passes locally.

Comment 4

[Jorg K (CEST = GMT+2)]

Thanks, but I don't think this is right at all. Have you tested this after refreshing your local build?

File.createFromNsIFile() is async now, so you need something like:
https://hg.mozilla.org/mozilla-central/rev/b6a45e1bab4c#l18.15
or perhaps:
https://hg.mozilla.org/mozilla-central/rev/b6a45e1bab4c#l17.12

I'm experimenting with a .then right now.

Comment 5

[alta88]

Comment on attachment 8835638 [details] [diff] [review]
date.patch


ah, of course..

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1338138-create-file-async.patch (v1)

This repairs the function in TB, the test still fails since it needs to wait differently now.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1338138-create-file-async.patch (v2).

This does the trick. The indentation is *deliberately* wrong for easier review. I'll indent it correctly when landing.

I put Aceman as a second reviewer, so whoever comes first. It would be good to get this landed quickly, since the function is currently broken.

I testes pasting from MS Word, too, since for that purpose we have all this processing.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8835698 [details] [diff] [review]
1338138-create-file-async.patch (v2).

Review of attachment 8835698 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2269,5 @@
>    let html = dataTransfer.getData("text/html");
>    let doc = (new DOMParser()).parseFromString(html, "text/html");
>    let tmpD = Services.dirsvc.get("TmpD", Components.interfaces.nsIFile);
>    let pendingConversions = 0;
> +  let firstTime = true;

I don't like the name. It's easier to understand too as a counter like it used to be

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #8)
> > +  let firstTime = true;
> I don't like the name. It's easier to understand too as a counter like it
> used to be
Well, the counter confused me. I was looking whether anyone looked at the value. No one. Can we leave it a bool? What name do you suggest?

needToCancelDefault = false; ?

Comment 10

[Jorg K (CEST = GMT+2)]

Sorry, needToCancelDefault = true; to start with.

Comment 11

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e5b59ae458f1c9620edad6c53fca4e9e511cb2d8
I left the bool, but changed the name and gave it a nice comment. The changeset looks terrible due to the changed indentation of a long block.

Thanks for the quick review, btw.

Comment 12

[alta88]

but you dropped the lastModifiedDate change, meaning someone will have to come back here..
https://dxr.mozilla.org/comm-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/mozilla/dom/webidl/File.webidl#33

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e5b59ae458f1c9620edad6c53fca4e9e511cb2d8 (comment #11)
https://hg.mozilla.org/comm-central/rev/af32a353227ef5633d06be9ffee8fd491af3350d (follow up)

Thanks, yes, I saw the deprecation warning but honestly, I couldn't see the other interface existed. Too many fires at once. Can you please point me to it. I landed this in your name since it was your idea in the first place. Thanks again.

Comment 14

[alta88]

It's |lastModified| and is in line 19 of the link in comment 12. It's also in the File spec:
https://w3c.github.io/FileAPI/#file

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks, I did see that but was confused by the |long long|, ah well, it's just a lot of milliseconds ;-)
All done now, one fire less ;-)