Closed
Bug 1338138
Opened 7 years ago
Closed 7 years ago
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-blocked-content.js | test-blocked-content.js::test_paste_file_urls
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 54.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
(Keywords: intermittent-failure, Whiteboard: [Thunderbird-testfailure: Z all])
Attachments
(1 file, 2 obsolete files)
3.00 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Whiteboard: [Thunderbird-testfailure] → [Thunderbird-testfailure: Z all]
Assignee | ||
Comment 1•7 years 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•7 years 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.
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•7 years 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 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•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
Sorry, needToCancelDefault = true; to start with.
Assignee | ||
Comment 11•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Comment 12•7 years 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•7 years 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•7 years 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•7 years 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.
Description
•