Closed Bug 1500420 Opened 3 years ago Closed 3 years ago

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


(Thunderbird :: General, defect)

Not set


(Not tracked)

Thunderbird 65.0


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



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


(2 files)

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

M-C last good: c0c288dc283e315982949e5d6565520f58
M-C first bad: 7d74c59053843bd79ed1456e830a760fdf 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
Flags: needinfo?(acelists)
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.
OK, after backing out bug 1492648 from M-C and bug 1500347 from C-C the test failures have disappeared.
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: " +

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.
Flags: needinfo?(acelists)
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

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


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
and no longer

as expected.

The failing check is here:

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).
Blocks: 1492648, 1500347
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(acelists)
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?
Flags: needinfo?(bzbarsky)
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
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.
Flags: needinfo?(kyle)
Summary: TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/attachment/test-attachment.js → Attachment name click does nothing, should open attachment - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/attachment/test-attachment.js
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:
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(acelists)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Pushed by
Temporarily disable part of failing test-attachment.js::test_attachment_name_click. rs=bustage-fix

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.
Flags: needinfo?(bzbarsky)
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:
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.
Assignee: nobody → jorgk
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(kyle)
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.
Flags: needinfo?(kyle)
(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:
> 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:

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.

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.
Flags: needinfo?(bzbarsky)
Pushed by
Backed out changeset bc99383f0a1c since bug 1492648 causing the failure was backed out. a=backout
> 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.
Flags: needinfo?(bzbarsky)
Will be fixed by using SetLoadType() in bug 1500347.
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → DUPLICATE
Target Milestone: --- → Thunderbird 65.0
Duplicate of bug: 1500347
You need to log in before you can comment on or make changes to this bug.