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

RESOLVED FIXED in Thunderbird 54.0

Status

Thunderbird
General
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

({intermittent-failure})

Trunk
Thunderbird 54.0
intermittent-failure

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Thunderbird-testfailure: Z all])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 months ago
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
(Assignee)

Updated

3 months ago
Whiteboard: [Thunderbird-testfailure] → [Thunderbird-testfailure: Z all]
(Assignee)

Comment 1

3 months ago
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.
(Assignee)

Comment 2

3 months ago
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

3 months ago
Created attachment 8835638 [details] [diff] [review]
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.
Attachment #8835638 - Flags: review?(jorgk)
(Assignee)

Comment 4

3 months ago
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

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


ah, of course..
Attachment #8835638 - Attachment is obsolete: true
Attachment #8835638 - Flags: review?(jorgk)
(Assignee)

Comment 6

3 months ago
Created attachment 8835662 [details] [diff] [review]
WIP: 1338138-create-file-async.patch (v1)

This repairs the function in TB, the test still fails since it needs to wait differently now.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 months ago
Created attachment 8835698 [details] [diff] [review]
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.
Attachment #8835662 - Attachment is obsolete: true
Attachment #8835698 - Flags: review?(mkmelin+mozilla)
Attachment #8835698 - Flags: review?(acelists)

Comment 8

3 months ago
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+
(Assignee)

Comment 9

3 months ago
(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; ?
(Assignee)

Comment 10

3 months ago
Sorry, needToCancelDefault = true; to start with.
(Assignee)

Comment 11

3 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0

Comment 12

2 months ago
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
(Assignee)

Comment 13

2 months ago
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

2 months ago
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
(Assignee)

Comment 15

2 months ago
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.