Page MenuHomeWildfire Games

Fix a compartment mismatch in XmppClient / Possibly fix semi-random lobby / in game JS crash
ClosedPublic

Authored by wraitii on Aug 2 2020, 12:12 PM.

Details

Summary

This is a possible fix for #5655.

While running SM45/SM52 in debug mode, it triggered compartment failures. Indeed, it seems XMPP is initialised with a ScriptInterface that isn't the same as the scriptInterface calling "guiPollNewMessages".

I couldn't reproduce the crash using P221 with this fix applied on SM52.

Test Plan

Try to reproduce the random JS crash.

Event Timeline

wraitii created this revision.Aug 2 2020, 12:12 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2838/display/redirect

wraitii edited the summary of this revision. (Show Details)Aug 2 2020, 12:14 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1201/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2300/display/redirect

wraitii requested review of this revision.Aug 2 2020, 12:17 PM
Itms added a comment.Aug 2 2020, 12:27 PM

The root of the issue might be around this part of the code, but I'm not convinced with the patch.

First of all you are changing from using the interface of the GUI manager (which should keep existing (maybe it doesn't and causes the crash, but I'm not convinced)) to using the interface of the current GUI page (line 98), which can be destroyed anytime. This doesn't make sense. Or maybe this is a leftover, because you do the correct change line 118.

You should adapt the patch to SVN. You are doing very strange stuff with Requests, they are supposed to be created/destroyed in a LIFO way so you should add braces somewhere.

Finally I am a bit afraid that maybe you removed the crash merely by removing entirely the chat messages code ?

wraitii updated this revision to Diff 13002.Aug 2 2020, 12:41 PM

SM45 compatible version and cleaner patch.

wraitii edited the summary of this revision. (Show Details)Aug 2 2020, 12:42 PM
Vulcan added a comment.Aug 2 2020, 1:00 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2839/display/redirect

Itms added a comment.Aug 2 2020, 1:05 PM

Ha, this makes far more sense, thank you! ?

Does it fix the crash (or maybe you can't reproduce it in the first place in SM45)? In any case this is a good change.

I think it would be nice to also replace the direct call to JS_StructuredClone line 734, which might be incorrect as well (maybe it's not, I'm just reading the diff and I don't have the rest of the code in front of me).

I am not against replacing the raw pointer by a unique_ptr like you did in the previous diff, but indeed I don't think that is relevant to the bug. If the raw pointer was an issue, the segfault trace would be different.

Itms added a comment.Aug 2 2020, 1:10 PM

By the way, a better name for the diff and for a future commit would be Fix rooting mistake in GuiPoll{New,Historic}Messages. But it would be great to add a fixes #5655, of course ? I'll rebase the branch upon this commit so that we can test if the crash is fixed.

(I realized I forgot to submit inlines highlighting flaws of my OG patch, sorry).

In D2922#126804, @Itms wrote:

Does it fix the crash (or maybe you can't reproduce it in the first place in SM45)? In any case this is a good change.

I couldn't get it to trigger on SM45. On SM52, I didn't have the crash with the patch, even if I had it regularly without on the same settings, but it was still rather spurious, so I won't confirm 100%.
What it does do however is fix a compartment mismatch assertion in debug mode in both SM45 and SM52.

I think it would be nice to also replace the direct call to JS_StructuredClone line 734, which might be incorrect as well (maybe it's not, I'm just reading the diff and I don't have the rest of the code in front of me).

Yeah I think it's not required, since this is all m_ScriptInterface.

I am not against replacing the raw pointer by a unique_ptr like you did in the previous diff, but indeed I don't think that is relevant to the bug. If the raw pointer was an issue, the segfault trace would be different.

Right, I think I just had a doubt originally. I don't think it's required.

wraitii retitled this revision from Possibly fix semi-random lobby / in game JS crash to Fix a compartment mismatch in XmppClient / Possibly fix semi-random lobby / in game JS crash.Aug 2 2020, 1:12 PM
wraitii edited the summary of this revision. (Show Details)

Additionally, given that:

  • It happens on the lobby MP gamesetup page
  • It doesn't happen on the regular MP gameosetup page

I think it's quite safe to say that xmpp has something to do with it.

Itms added a comment.Aug 2 2020, 1:16 PM

Yes, the more I look at it the more I'm convinced this is the issue. I really can't do the rebase today but should be possible to see whether the crash is fixed in the upcoming days. ?

Stan added a subscriber: Stan.Aug 2 2020, 10:33 PM

Fixes the issue for me in Debug mode SM45, (I had to rebuild gloox, as it was triggering unrelated assertions see IRC from today)

Itms added a comment.Sep 17 2020, 2:39 PM

Hi there! I rebased https://github.com/na-Itms/0ad/tree/spidermonkey upon this. Please test it, along SM52 ? I'll update the forum post whenever I have the chance.

Just tested -> couldn't reproduce the crash via launching a map or triggering a shrinking GC on the lobby page.

Itms accepted this revision.Oct 28 2020, 6:55 PM

The code change is correct and, apparently, works under SM45 and SM52.

I can commit it if you are away.

If you commit it yourself, don't forget to mention the commit that added the code (rP22856), as per elexis' request. I would also remove the capitalization of Interface in both comment lines (just a nitpick).

This revision is now accepted and ready to land.Oct 28 2020, 6:55 PM
In D2922#133604, @Itms wrote:

I can commit it if you are away.

Go ahead, haven't found the motivation to go back to committing just yet and we should maximise on available dev time :)

Thanks for the review and thanks for working on SM52 !