1374779 - Clicking on addbook: link from vCard displayed inline hangs TB

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: attached VCard.eml

Clicking on addbook: link from vCard displayed inline hangs TB.

STR:
Set attachments to display inline.
Import and open attached message.
Click on the vCard link.

Result:
TB hangs, menu unresponsive.

Problem doesn't happen in TB 52 ESR, but does happen in TB 55 and 56.

Alice, can you find the regression, please.

Comment 1

[Alice0775 White]

Where is "vCard link"?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot showing the link

When hovering the (missing) icon.

Comment 3

[Alice0775 White]

regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=c2e8d98586613f94a9e8217c9435a30cc3459a61&tochange=7f8fe6bf3004d4968569227b049ae4a1e31affde
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=57ac9f63fc6953f4efeb0cc84a60192d3721251f&tochange=f13abb8ba9f366c9f32a3146245adf642528becd

Comment 4

[Alice0775 White]

regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=54554cf58747b820f72730520a148d8021e08418&tochange=7f8fe6bf3004d4968569227b049ae4a1e31affde
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=57ac9f63fc6953f4efeb0cc84a60192d3721251f&tochange=f13abb8ba9f366c9f32a3146245adf642528becd

Comment 5

[Alice0775 White]

Error Console shows
Security Error: Content at mailbox:///C:/Users/fuku/AppData/Roaming/Thunderbird/Profiles/em4ra9pm.gfff/Mail/pop.mail.yahoo.co.jp/Inbox?number=2 may not load or link to chrome://messenger/content/addressbook/abNewCardDialog.xul.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks, Alice!!!

Boris, looks like some of your security changes broke a piece in TB, addbook: links when clicked in a message.

Here's some debug from out content policy code:

=== nsMsgContentPolicy::ShouldLoad: URI=|addbook:add?action=add?vcard=begin%3Avcard%0D%0Aversion%3A3.0%0D%0Afn%3AMeister%0D%0Aemail%3BTYPE%3DWORK%2CPREF%3Ameister%40web.de%0D%0Aend%3Avcard%0D%0A%0D%0A|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/June%202017?number=11|
=== returning (4) 1

My debug code is this:
  if (IsExposedProtocol(aContentLocation))
  {
    *aDecision = nsIContentPolicy::ACCEPT;
    printf("=== returning (4) %d\n", (int) *aDecision); return NS_OK;
  }
and the IsExposedProtocol() function is just returning "accept" (1) for addbook: links.

Alice has found something interesting, see previous comment. I don't know how he did, since for me, when I click, all windows are blocked.

So somehow that content wants to trigger a load on abNewCardDialog.xul which is exactly the dialogue that will prompt you to add the card. Looks like you tightened security there.

So what can we do to resolve this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I didn't make any security changes in the range linked in comment 4.

> So what can we do to resolve this?

Find the actual changeset that regressed this and ask the people involved in it.  In this case, chances are that's the fix for bug 1182569, given that range.

Comment 8

[Jorg K (CEST = GMT+2)]

Of course, if I CC'ed Ben Bucksch here, he'd tell us that the UI is conceptually wrong since content shouldn't be able to trigger commands, like he did in bug 1372411 comment #0. However, links have traditionally triggered "commands", like clicking a mailto: link opens a compose window.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That said, given the error message from comment 5, it sounds like the abNewCardDialog.xul load is happening as if triggered by the untrusted "mailbox:///C:/Users/fuku/AppData/Roaming/Thunderbird/Profiles/em4ra9pm.gfff/Mail/pop.mail.yahoo.co.jp/Inbox?number=2" url, and the load gets blocked.  Presumably this is coming from something like nsAbContentHandler::HandleContent doing a window.open in C++ using the content window, not the chrome, as the parent.  If so, changing it to use the chrome window there might help.

Comment 10

[Jorg K (CEST = GMT+2)]

Yes, I assume it's coming from the nsAbContentHandler::HandleContent code. Where do I get a chrome window? Secondly, I can't call OpenDialog() on nsIDOMChromeWindow.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Where do I get a chrome window? 

That depends on the structure of your UI, but presumably if you're running all this in-process you could get the same-type root docshell and then get its parent and then get its window.

> Secondly, I can't call OpenDialog() on nsIDOMChromeWindow.

You don't need an nsIDOMChromeWindow.  You need the nsIDOMWindow for the Thunderbird UI, as opposed to the nsIDOMWindow for the vcard being viewed.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #11)
> That depends on the structure of your UI, but presumably if you're running
> all this in-process you could get the same-type root docshell and then get
> its parent and then get its window.
Could you please write down the function names to achieve this or a link to a sample.

As you know, we TB people are generalists, Jacks of all trades, masters of none. (Recently I did bug 1363281 and I'm still dealing with follow-up bugs, so I think it's fair to say that I know more about encodings than docshell.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Could you please write down the function names to achieve this or a link to a sample.

No, because it depends on the exact structure of your chrome, and I don't know what that is.

That said, you could try what I suggested in comment 11 or a modified version: get the docshell from the window (see nsPIDOMWindow::GetDocshell), GetRootTreeItem on the docshell to get the root-most thing, which is presumably system-principaled, do_GetInterface an nsIDOMWindow from that.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1374779-addbook-link.patch (v1)

Thank you, this works, and I don't think I can find a better reviewer than you.

BTW,
            nsCOMPtr<nsIDOMWindow> window(do_GetInterface(root));
            rv = window->OpenDialog( ...
gave a compile error:
'OpenDialog': is not a member of 'nsIDOMWindow'

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8880011 [details] [diff] [review]
1374779-addbook-link.patch (v1)

r=me

Comment 16

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4e64a09ef23545c60be574f055c892a06a537344

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8880011 [details] [diff] [review]
1374779-addbook-link.patch (v1)

We need to fix this in the beta version, too, since it already regressed in bug 1182569 in mozilla53.

Comment 18

[Jorg K (CEST = GMT+2)]

Beta (TB 55):
https://hg.mozilla.org/releases/comm-beta/rev/50abbd56a167