Closed Bug 1338138 Opened 4 years ago Closed 4 years ago

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


(Thunderbird :: General, defect)

Not set


(Not tracked)

Thunderbird 54.0


(Reporter: jorgk-bmo, Assigned: jorgk-bmo)


(Keywords: intermittent-failure, Whiteboard: [Thunderbird-testfailure: Z all])


(1 file, 2 obsolete files)

First seen: Thu Feb 9, 2017, 7:47:00

Last good: M-C: 5e17f9181c6c
First bad: M-C: 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
Whiteboard: [Thunderbird-testfailure] → [Thunderbird-testfailure: Z all]
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.
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

Let me see whether I find a good example for a replacement.
Attached patch date.patch (obsolete) — Splinter Review
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.
Attachment #8835638 - Flags: review?(jorgk)
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:
or perhaps:

I'm experimenting with a .then right now.
Comment on attachment 8835638 [details] [diff] [review]

ah, of course..
Attachment #8835638 - Attachment is obsolete: true
Attachment #8835638 - Flags: review?(jorgk)
This repairs the function in TB, the test still fails since it needs to wait differently now.
Assignee: nobody → jorgk
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.
Attachment #8835662 - Attachment is obsolete: true
Attachment #8835698 - Flags: review?(mkmelin+mozilla)
Attachment #8835698 - Flags: review?(acelists)
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
Attachment #8835698 - Flags: review?(mkmelin+mozilla)
Attachment #8835698 - Flags: review?(acelists)
Attachment #8835698 - Flags: review+
(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; ?
Sorry, needToCancelDefault = true; to start with.
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.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
but you dropped the lastModifiedDate change, meaning someone will have to come back here.. (comment #11) (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.
It's |lastModified| and is in line 19 of the link in comment 12. It's also in the File spec:
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 ;-)
You need to log in before you can comment on or make changes to this bug.