1500420 - Attachment name click does nothing, should open attachment - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/attachment/test-attachment.js

Status: RESOLVED DUPLICATE of bug 1500347

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_name_click
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_list_expansion

First seen Fri, Oct 19, 2018, 10:33:43
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=3a91ffa3617c611a043c9df09546a06d71831cd7

M-C last good: c0c288dc283e315982949e5d6565520f58
M-C first bad: 7d74c59053843bd79ed1456e830a760fdf
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c0c288dc283e315982949e5d6565520f58&tochange=7d74c59053843bd79ed1456e830a760fdf

https://taskcluster-artifacts.net/I0eLVVnNTrygJ4yuUe2OKQ/0/public/logs/live_backing.log gives the same errors as running the test locally:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\attachment\test-attachment.js | test-attachment.js::test_attachment_name_click
  EXCEPTION: Timeout waiting for modal dialog to open.
    at: utils.js line 396
       TimeoutError utils.js:396 13
       waitFor utils.js:452 11
       WindowWatcher_waitForModalDialog test-window-helpers.js:386 5
       wait_for_modal_dialog test-window-helpers.js:619 3
       test_attachment_name_click test-attachment.js:246 3

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\attachment\test-attachment.js | test-attachment.js::test_attachment_right_click_single
  EXCEPTION: The content pane is not displaying the right message! Should be: mailbox:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2 but it's: mailbox:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt
    at: test-folder-display-helpers.js line 2084
       _internal_assert_displayed test-folder-display-helpers.js:2084 13
       assert_selected_and_displayed test-folder-display-helpers.js:2138 3
       test_attachment_right_click_single test-attachment.js:268 3

Comment 1

[Jorg K (CEST = GMT+2)]

The regression range is not conclusive. If could be bug 1492648 which just go backed out, so I'll back out my port from bug 1500347 and then we'll see.

Comment 2

[Jorg K (CEST = GMT+2)]

OK, after backing out bug 1492648 from M-C and bug 1500347 from C-C the test failures have disappeared.

Comment 3

[Jorg K (CEST = GMT+2)]

Expected: mailbox:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2
Actual:   mailbox:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt

So some behaviour has changed and we got a slightly different URL returned, this time with a full query part.

It comes from this code:
      throw new Error("The content pane is not displaying the right message! " +
                      "Should be: " + msgUrl.spec + " but it's: " +
                      troller.window.content.location.href);

So somehow window.content.location.href which I believe should only return the message URL
  mailbox:// .... ?number=2
now returns something else which does not really address the message but a message part.

Comment 4

[Jorg K (CEST = GMT+2)]

Bug 1492648 and bug 1500347 have landed again and the test failures have re-appeared. This time they are:

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1540548542/build/tests/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_name_click
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1540548542/build/tests/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_right_click_single

https://taskcluster-artifacts.net/ZmxLK1sFRRWSmJNijIByNw/0/public/logs/live_backing.log

This time the errors are:

INFO -  SUMMARY-UNEXPECTED-FAIL | test-attachment.js | test-attachment.js::test_attachment_name_click
INFO -    EXCEPTION: Timeout waiting for modal dialog to open.
INFO -      at: utils.js line 396
INFO -         TimeoutError utils.js:396 13
INFO -         waitFor utils.js:452 11
INFO -         WindowWatcher_waitForModalDialog test-window-helpers.js:386 5
INFO -         wait_for_modal_dialog test-window-helpers.js:619 3
INFO -         test_attachment_name_click test-attachment.js:246 3

and

INFO -  SUMMARY-UNEXPECTED-FAIL | test-attachment.js | test-attachment.js::test_attachment_right_click_single
INFO -    EXCEPTION: The content pane is not displaying the right message! Should be: mailbox:///Users/cltbld/tasks/task_1540548542/build/tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2 but it's: mailbox:///Users/cltbld/tasks/task_1540548542/build/tests/mozmill/mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt
INFO -      at: test-folder-display-helpers.js line 2084
INFO -         _internal_assert_displayed test-folder-display-helpers.js:2084 13
INFO -         assert_selected_and_displayed test-folder-display-helpers.js:2138 3
INFO -         test_attachment_right_click_single test-attachment.js:268 3

So looks like the message pane URL is now
.../mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt
and no longer
.../mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2

as expected.

The failing check is here:
https://searchfox.org/comm-central/rev/c7a8bbb19fbc5bd8edba7a775de70aade7b25301/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#2084

So window.content.location.href as brought back a different URL from what it use to return.

Kyle and Boris, is that expected? Also see bug 1492648 comment #9 (which sadly received no attention).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Where is the relevant load coming from to start with?  Please attach (not paste) the C++ and JS stacks.  What URI is it passing to the relevant docshell?  Have we checked whether location.href changed what it's reporting  even though the same thing is being loaded, or whether a different URL is being loaded now?

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks for your comment, Boris. Too hard to debug ... :-( - But please read on:

OK, I worked out what happens. If you have a single attachment and the attachment pane is collapsed, clicking on its name opens the attachment. In case of the test, we're waiting for a modal dialogue that will say "unknown content type". That doesn't work any more and nothing happens. Hence the failure in subtest "test_attachment_name_click".

The other failure is a consequence of the first, so taking the first test out makes the second one pass.

Clicking the attachment now gives "Loading Message..." in the status bar, so instead of loading the attachment
.../mozmillprofile/Mail/Local%20Folders/AttachmentA?number=2&part=1.2&filename=ubik.txt&type=text/plain&filename=ubik.txt
we load the message. So something is going wrong with loading that URL.

Looks like when selecting the message later, we get the requesting URL back, hence the failure in the second test.

I'll investigate how and were we load the attachment.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1500420-disable-part-of-test.patch

Comment 8

[Jorg K (CEST = GMT+2)]

I assume there aren't many tests opening attachments since that would launch all sorts of programs not under the control of TB. So this might be the only test doing so and it uses an unknown type to check for the internal "unknown type" dialogue.

Opening the attachment happens here:
https://hg.mozilla.org/comm-central/rev/fb0fff3cd0f7#l4.57
so maybe our dirty little trick doesn't work any more. Or we need to pass in other flags?

May I trouble you again, Boris, although I didn't answer your questions.

Comment 9

[Jorg K (CEST = GMT+2)]

Wow, behaviour has changed here :-( - If I click a picture attachment, the content is now loaded into the message preview :-( - That would explain why selecting the message now brings back the "part URL", because that *was* actually loaded.

So I don't know what I can do about this, M-C behaviour has certainly changed.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bc99383f0a1c
Temporarily disable part of failing test-attachment.js::test_attachment_name_click. rs=bustage-fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jorg,

Thank you, that link from comment 8 helps a lot.

I think the behavior of LoadURI() did change.  The old version used to do:

  uint32_t loadType = MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadFlags);

The equivalent in the new world would be:

  aLoadState->SetLoadType(MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadState->LoadFlags()));

or so but I don't see that being done anywhere.  The upshot is that the LOAD_FLAGS_IS_LINK flag is getting lost, which affects load targeting later, I expect.

We should presumably put that code back into LoadURI.  Alternately,  we could ask the callers to do it, but seems like this is a docshell implementation detail...

Anyway, you can try doing that SetLoadType bit in nsMailboxService::OpenAttachment locally as a test to see whether it fixes things.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1500420-follow-up.patch

Thanks Boris, that fixes the test.

I'm just wondering whether the other call sites should receive the same treatment. Please look for LOAD_FLAGS_IS_LINK in this changeset:
https://hg.mozilla.org/comm-central/rev/fb0fff3cd0f7
There are five call sites that use LOAD_FLAGS_IS_LINK.

Or do you want to revert the behaviour change in M-C.

Hmm, no f? requests currently.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Hmm, no f? requests currently.

Yes, because I'm traveling and way behind on everything....

> Or do you want to revert the behaviour change in M-C.

I suspect that's what we should do, but would like Kyle to double-check that this was not an intentional change.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yup. This is backout-and-reland material. This already happened once with this patch, so I can't say I'm surprised it's happened again. Hopefully this should be an easy fix though.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Jorg K (GMT+2) from comment #12)
> Created attachment 9020484 [details] [diff] [review]
> 1500420-follow-up.patch
> 
> Thanks Boris, that fixes the test.
> 
> I'm just wondering whether the other call sites should receive the same
> treatment. Please look for LOAD_FLAGS_IS_LINK in this changeset:
> https://hg.mozilla.org/comm-central/rev/fb0fff3cd0f7
> There are five call sites that use LOAD_FLAGS_IS_LINK.
> 
> Or do you want to revert the behaviour change in M-C.
> 
> Hmm, no f? requests currently.

Ok, well, I may have backed out too soon. I reverted because this sounded like another bug that no tests caught that I'd been fighting for a week before landing, but it turns out, this is different.

On further inspection, I'm not sure this should have been reverted in m-c, and I think either call sites should be required to set up this information themselves, to make this uniform across m-c and c-c, or we should talk about what this should look like across both code bases.

Here's how LoadURI looked before my patch:

https://searchfox.org/mozilla-central/rev/96d721e85144ef9a190d20a00fb136f0468341b1/docshell/base/nsDocShell.cpp#645

The interesting parts here are the signature, and line 696 under it. aLoadInfo* is a pointer, but we allow it to be null.

There is nowhere in m-c where aLoadInfo is passed as nullptr. Every callsite for LoadURI does LoadType setup *before* calling LoadURI, meaning that the line pointed out in Comment 11 was effectively useless, because we'd set the LoadType to a default, then instantly reset it when we broke the members out of the LoadInfo object. Obviously I didn't look at the c-c call sites for this.

My patch added an assert for null LoadState checking, and removed the conditional block where we break out the LoadInfo into local variables since we're just using that object now. That removal included the default LoadType setup, because everyone passed in their own anyways.

https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/docshell/base/nsDocShell.cpp#648

If we *do* want to continue to have a default LoadType setup in LoadURI for LoadState, we need some sort of default value to set to on LoadState construction, that lets it know to do the LOAD_NORMAL generation in Comment 11. Since the patch for bug 1492648 is already backed out, I could go ahead and try to add that now before relanding. Let me know your thoughts.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/078e0cf338b5
Backed out changeset bc99383f0a1c since bug 1492648 causing the failure was backed out. a=backout

Comment 17

[Boris Zbarsky [:bzbarsky]]

> There is nowhere in m-c where aLoadInfo is passed as nullptr.

Aha.  OK, that explains why this wasn't an issue on m-c.  Thank you for hunting it down!

I would be fine with just saying (and documenting) that callers should set the load type in the load state they're passing in.  c-c can pass LOAD_LINK in all the places where they currently use nsIWebNavigation::LOAD_FLAGS_IS_LINK, and that should make things work as intended.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, relanded at https://hg.mozilla.org/integration/mozilla-inbound/diff/33a2f916d13c/docshell/base/nsDocShell.cpp. I think this is closable once bug 1500347 comes in?

Comment 19

[Jorg K (CEST = GMT+2)]

Will be fixed by using SetLoadType() in bug 1500347.