870864 - nsImapProtocol uses wrapped JS objects off the main thread

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=22823512&tree=Thunderbird-Trunk

Comment 1

[Bobby Holley (:bholley)]

This stuff needs to use nsMainThreadPtrHolder. See the dependent bugs of bug 773610 for examples.

Comment 2

[Josh Matthews [:jdm]]

It's possible that the problem is the nsIMsgWindow nsCOMPtr in TryToLogon, since I see various tests implementing JS versions of that.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

There were mozmill crashes on c-c too. Don't know if they had the same root cause or not.

Comment 4

[Joshua Cranmer [:jcranmer]]

I think all the uses of the msg window are on the main thread, but I'm testing a patch which uses the wrapper right now. Damn IDL...

Comment 5

[Josh Matthews [:jdm]]

TryToLogon is used off the main thread and it obtains the nsIMsgWindow interface into an nsCOMPtr which would trigger the abort if it's implemented by a JS object.

Comment 6

[Joshua Cranmer [:jcranmer]]

When I said "use", I meant any actual calls beyond implicit AddRef/Release (the *sink calls is also synchronously proxied back to the main thread). Since I pulled from the m-c just after the backout had been applied, I haven't verified that the crash is fixed yet.

Comment 7

[Josh Matthews [:jdm]]

I tried to dig in, but it appeared more complicated than I originally anticipated. Joshua, I'm hoping to reland the original abort in the next couple days, so it would be great if you could tackle this.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: May fix the issue

I don't know if this actually stops the assertion failure or not.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Make IMAP protocol avoid addreffing potentially non-threadsafe windows off the main thread.

Comment 10

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=101318bf16cf

Comment 11

[[e-mail]]

IMHO this is the wrong fix; all the IMAP tests should be using real nsMsgWindow objects. The openFolder attribute isn't even readonly!

Comment 12

[[e-mail]]

(We could fix the alerts test and make nsIMsgWindow builtinclass but that would be harder and probably deserves its own bug.)

Comment 13

[Josh Matthews [:jdm]]

Attachment: Disable tests until a proper fix is provided.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Disable tests until a proper fix is provided.

Comment 15

[[e-mail]]

Comment on attachment 760949 [details] [diff] [review]
Disable tests until a proper fix is provided.

> [test_imapAttachmentSaves.js]
>+# Disabled until bug 870864 is resolved
>+skip-if = true
...
> [test_imapHdrChunking.js]
>+# Disabled until bug 870864 is resolved
>+skip-if = true
I'd spotted these tests misusing nsIMsgWindow but I hadn't realised that there were so many others :-(

> [test_offlineStoreLocking.js]
>-# Doesn't currently work on Windows, bug 782625
>-skip-if = os == "win"
>+# Disabled until bug 870864 is resolved
>+skip-if = true
Please leave the Windows comment in, just in case someone erroneously tries to enable it on all platforms.

Comment 16

[Josh Matthews [:jdm]]

https://hg.mozilla.org/comm-central/rev/564f827c6869

Comment 17

[Mark Banner (:standard8)]

Setting as tracking-thunderbird24+. I'm uneasy with the idea of a release with all these tests disabled.

Comment 18

[Josh Matthews [:jdm]]

Any such release will be crashing regularly when using IMAP, fwiw, so there should be little chance of that happening.

Comment 19

[Josh Matthews [:jdm]]

Actually, that's wrong. I forgot that only the tests use the JS-implemented message window, not the actual app.

Comment 20

[Josh Matthews [:jdm]]

Anybody else want to take the review here?

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 754606 [details] [diff] [review]
Make IMAP protocol avoid addreffing potentially non-threadsafe windows off the main thread.

Neil, can you take a look at this?

Comment 22

[[e-mail]]

Comment on attachment 754606 [details] [diff] [review]
Make IMAP protocol avoid addreffing potentially non-threadsafe windows off the main thread.

I already did, see comment #11.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 754606 [details] [diff] [review]
Make IMAP protocol avoid addreffing potentially non-threadsafe windows off the main thread.

Sorry for not really looking earlier. I think we need to fix the tests as Neil suggests, I'm not quite sure that's very easy, but it seems the better fix in the long run.

Comment 24

[Josh Matthews [:jdm]]

Let's not pretend that I'm actually working on this.

Comment 25

[Wayne Mery (:wsmwk)]

kent, can you suggest someone who might be able to work on this?

Comment 26

[Kent James (:rkent)]

Attachment: 870864.patch

If the approach in comment 11 can be done this way, then perhaps this will fix this.

Otherwise either Joshua or Neil would probably be good candidates to try to fix the msgWindow. But if this is only a testing issue, I would prefer we just fix the testing issue.

Comment 27

[[e-mail]]

Comment on attachment 8541304 [details] [diff] [review]
870864.patch

I'm not sure you need to override authPrompt because the login manage provides the default auth prompt anyway.

I'm not even sure you need to override the prompt dialog, since alertUtilsPrompts already overrides both the window watcher and the prompt service.

You obviously don't need to override them in the IMAP case. Does the POP case actually try to access them off the main thread?

But if you do need to override them, I'd prefer if you added a testing interface with the method to override the prompt dialog.

Comment 28

[Kent James (:rkent)]

With a few additional fixes, I don't need to override them, with one exception:

In test_imapPasswordFailure, the password dialog fails due to this code in nsMsgIncomingServer:

  if (m_password.IsEmpty())
  {
    nsCOMPtr<nsIAuthPrompt> dialog;
    // aMsgWindow is required if we need to prompt
    if (aMsgWindow)
    {
      rv = aMsgWindow->GetAuthPrompt(getter_AddRefs(dialog));
      NS_ENSURE_SUCCESS(rv, rv);
    }

which is calling:

NS_IMETHODIMP nsMsgWindow::GetAuthPrompt(nsIAuthPrompt * *aAuthPrompt)
{
  NS_ENSURE_ARG_POINTER(aAuthPrompt);
  if (!mRootDocShellWeak)
    return NS_ERROR_FAILURE;

Any suggestions of how to fix that with the override of authPrompt?

Comment 29

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #28)
> 
> Any suggestions of how to fix that with the override of authPrompt?

Comment 30

[[e-mail]]

In the POP case since it's main-thread only are we just using a dummy msgWindow object which therefore can provide a dummy auth prompt?

Comment 31

[Joshua Cranmer [:jcranmer]]

Barring that, it may be possible to make the GetAuthPrompt method construct a new nsIAuthPrompt like the dummy message window did in the case that no docshell is available.

Comment 32

[Kent James (:rkent)]

I'm going to set myself as Assigned because I have a patch proposal here that I should look at some day, but I don;t make any claims to be actually working on this at the moment. If you are competent feel free to take this.

Comment 33

[Wayne Mery (:wsmwk)]

Kent, 
The imap area is rather fragile because of the lingering off main thrad issues. Can this be added to the bottom of your top priority stack?  Or suggest somone else?

clearing - neil replied.

Comment 34

[Kent James (:rkent)]

Wayne, there are just too many other things for me to work on between now and release of Thunderbird 45. Magnus would be another possibility, but he also already is doing alot.

Comment 35

[Jorg K (CEST = GMT+2)]

When adding a new test to mailnews/imap/test/unit/xpcshell.ini for bug 629738 I came across this bug which disables a bunch of tests. So what's the status of this? Your patch is really minimal. Does it solve the problem?

Comment 36

[Kent James (:rkent)]

Attachment: Use a real window, add a test interface as Neal suggested

Yes I believes it works, here is an updated patch with Neal's suggestion.

Comment 37

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #36)
> Yes I believes it works, here is an updated patch with Neal's suggestion.
Great. Thanks. Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=59186d0b7944

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791395 [details] [diff] [review]
Use a real window, add a test interface as Neal suggested

Review of attachment 8791395 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please disable the test that also fails on Mac. See try run.

::: mailnews/imap/test/unit/xpcshell.ini
@@ +63,5 @@
>  [test_offlinePlayback.js]
>  run-sequentially = Starts server twice.
>  [test_offlineStoreLocking.js]
>  # Doesn't currently work on Windows, bug 782625
> +skip-if = os == 'win'

Please check the try run to see that this also fails on Mac.

Comment 39

[Kent James (:rkent)]

http://hg.mozilla.org/comm-central/rev/b133e1d1134c