1542397 - TEST-UNEXPECTED-FAIL | comm/chat/modules/test/test_filtering.js | xpcshell return code: -11

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | comm/chat/modules/test/test_filtering.js | xpcshell return code: -11
PROCESS-CRASH | comm/chat/modules/test/test_filtering.js | application crashed [@ mozilla::places::History::History()]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=61540ab18c9b1def917e2799b4a468c12a&tochange=7775ced80e99c60f1231b0b463e07f4da7

Looks like:
c36ad62407f3 Gijs Kruitbosch — Bug 1540170 - release assert if something tries to start the history service before profile startup, r=mak
c2298e57e803 Gijs Kruitbosch — Bug 1540170 - don't use history for windowless browsers, r=farre

Running the test locally confirms the crash:
0:01.45 pid:2812 Assertion failure: dirsvc && ((bool)(__builtin_expect(!!(!NS_FAILED_impl(dirsvc->Has("ProfD", &haveProfile))), 1))) && haveProfile (Can't construct history service if there is no profile.), at c:/mozilla-source/comm-central/toolkit/components/places/History.cpp:1456
0:02.33 pid:2812 #01: mozilla::xpcom::CreateInstanceImpl (c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\xpcom\components\StaticComponents.cpp:9513)

I tried running this in the debugger but I saw no JS stack.

I know, it's not popular, but I'll have to switch this off until it's fixed.

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/056054f30e9f
temporarily switch off Chat's test_filtering.js. rs=bustage-fix

Comment 2

[Jorg K (CEST = GMT+2)]

Chat still seems to be working, so switching off the test isn't hiding a complete failure :-)

Comment 3

[Patrick Cloke [:clokep]]

Florian and I talked a bit about this on IRC:

1:14:31 PM - florian: it's a fun one :)
1:14:46 PM - florian: I would be more interested in the C++ stack than in the JS stack
1:15:19 PM - clokep: "fun one" doesn't bode well. ;)
1:15:19 PM - florian: so 2 things here
1:15:46 PM - florian: 1. We can silence the failure by initializing the dirsvc with fake stuff in the xpcshell test.
1:16:25 PM - florian: 2. This is highlighting an actual perf bug! It looks like https://searchfox.org/comm-central/source/chat/modules/imContentSink.jsm#372 tries to init Places
1:18:50 PM - clokep: Oooo. I was afraid this was highlighting a security bug.
1:18:58 PM - clokep: Glad it is just a performance bug.
1:19:08 PM - clokep: Is it possible to have that not init places?
1:19:17 PM - florian: I have no idea
1:19:29 PM - florian: this is why I would like to see the C++ stack
1:19:57 PM - florian: clokep: that test being disabled opens the door to security issues being introduced and going under the radar
1:20:52 PM - florian: I was already frowning at that area of the code for performance reasons because that line creates a new DOM document for each message we sanitize... and creating a document now executes Custom Element JS code...
1:22:47 PM - clokep: That sounds inconvenient.

Not sure of the way forward however.

Comment 4

[Jorg K (CEST = GMT+2)]

C++ stack:

 0:01.75 pid:7104 Assertion failure: dirsvc && ((bool)(__builtin_expect(!!(!NS_FAILED_impl(dirsvc->Has("ProfD", &haveProfile))), 1))) && haveProfile (Can't construct history service if there is no profile.), at c:/mozilla-source/comm-central/toolkit/components/places/History.cpp:1456
 0:02.56 pid:7104 #01: mozilla::xpcom::CreateInstanceImpl (c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\xpcom\\components\\StaticComponents.cpp:9983)
 0:02.58 pid:7104 #02: nsComponentManagerImpl::GetServiceLocked (c:\\mozilla-source\\comm-central\\xpcom\\components\\nsComponentManager.cpp:1387)
 0:02.58 pid:7104 #03: nsComponentManagerImpl::GetServiceByContractID (c:\\mozilla-source\\comm-central\\xpcom\\components\\nsComponentManager.cpp:1574)
 0:02.59 pid:7104 #04: nsGetServiceByContractID::operator() (c:\\mozilla-source\\comm-central\\xpcom\\components\\nsComponentManagerUtils.cpp:244)
 0:02.61 pid:7104 #05: mozilla::places::History::GetService (c:\\mozilla-source\\comm-central\\toolkit\\components\\places\\History.cpp:1924)
 0:02.62 pid:7104 #06: mozilla::dom::Link::LinkState (c:\\mozilla-source\\comm-central\\dom\\base\\Link.cpp:369)
 0:02.64 pid:7104 #07: mozilla::dom::Document::FlushPendingLinkUpdates (c:\\mozilla-source\\comm-central\\dom\\base\\Document.cpp:9206)
 0:02.65 pid:7104 #08: mozilla::detail::RunnableMethodImpl<mozilla::dom::Document *,void (mozilla::dom::Document::*)(),1,mozilla::RunnableKind::Standard>::Run (c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\dist\\include\\nsThreadUtils.h:1177)
 0:02.67 pid:7104 #09: IdleRunnableWrapper::Run (c:\\mozilla-source\\comm-central\\xpcom\\threads\\nsThreadUtils.cpp:331)
 0:02.69 pid:7104 #10: nsThread::ProcessNextEvent (c:\\mozilla-source\\comm-central\\xpcom\\threads\\nsThread.cpp:1167)
 0:02.70 pid:7104 #11: nsThreadManager::SpinEventLoopUntilEmpty (c:\\mozilla-source\\comm-central\\xpcom\\threads\\nsThreadManager.cpp:519)
 0:02.70 pid:7104 #12: XPTC__InvokebyIndex[c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\dist\\bin\\xul.dll +0x838e5a2]
... and more.

Somehow this hits mozilla::places::History::GetService which complains the crashing way :-(

As I said in comment #0, I say now JS stack in the debugger.

Comment 5

[Florian Quèze [:florian]]

Marco, do you know if there's a way to avoid triggering the code path in the stack of comment 4 when running something like
(new DOMParser()).parseFromString("<span>" + aText + "</span>", "text/html")

In this case aText is a chat message received from the network that may contain links and other markup.

Comment 6

[Marco Bonardo [:mak]]

I don't think currently it's possible to disable link coloring for a specific document, though I don't see why this test couldn't create a profile. if it's xpcshell it may just use do_get_profile(), I think?

Comment 7

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] (Away 22 -28 Apr) from comment #6)

Thanks!

I don't think currently it's possible to disable link coloring for a specific document

Oh, is it link coloring that triggers this initialization? I would not have expected link coloring to play a role in documents that will never be displayed.

if it's xpcshell it may just use do_get_profile(), I think?

We can do this to silence the failure, yes. My question was because I was wondering if the failure might be uncovering something seriously inefficient in the way Thunderbird handles chat messages (this is the code that parses incoming markup to sanitize undesired elements or attributes, and then serializes it again).

Comment 8

[Patrick Cloke [:clokep]]

Hey Florian, do you know what the next step is here? It would be nic to get this test re-enabled.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This might not be the best way to do this, but I appreciate having this test enabled.

I also took the opportunity to comment a bunch of the related code and expand the tests a little.

I had to actually fix a test as well, I think this is due to changes in the CSS engine at some point in the past.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9168931 [details] [diff] [review]
Patch v1

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

::: chat/modules/imContentSink.jsm
@@ +262,5 @@
> +/**
> + * Removes nodes, attributes and styles that are not allowed according to the
> + * given rules.
> + *
> + * @param aNode           A DOM node to inspect recursively against the rules.

Yay for documentation. I guess this function is the only place using these "modes" so maybe best to ducument them here?

@param {Node} aNode - A Dom node....
@param {Object[]} aRules - .....
@param {string[]} aRules[].attrs - Each property of attrs is the name of an attribute.... 
@param {string[]} aRules[].tags - Each property of tags works identically to att...

... and so on

Comment 11

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #10)

Comment on attachment 9168931 [details] [diff] [review]
Patch v1

Review of attachment 9168931 [details] [diff] [review]:

::: chat/modules/imContentSink.jsm
@@ +262,5 @@

+/**

• • Removes nodes, attributes and styles that are not allowed according to the
• • given rules.
• • @param aNode A DOM node to inspect recursively against the rules.

Yay for documentation. I guess this function is the only place using these
"modes" so maybe best to ducument them here?

@param {Node} aNode - A Dom node....
@param {Object[]} aRules - .....
@param {string[]} aRules[].attrs - Each property of attrs is the name of an
attribute....
@param {string[]} aRules[].tags - Each property of tags works identically to
att...

... and so on

It is only used for this function, yes. I originally had the documentation here but wasn't sure how to document an object with particular parameters. Thanks for the hint! I'll rework it and hope it doesn't get too messy.

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Patch v2

This changed enough it is probably worth getting a second set of eyes on.

I only updated comments between the two patches, but I realized there actually was some documentation for these rulesets that I had missed. I converted it to jsdoc and combined it with my previous comments + took into account Magnus' feedback.

I don't think it is perfect, but I think it is much better!

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9169246 [details] [diff] [review]
Patch v2

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

::: chat/modules/imContentSink.jsm
@@ +234,5 @@
> + * A dynamic rule which decides if an attribute is allowed base on the
> + * attribute's value.
> + *
> + * @callback  ValueRule
> + * @param {string} value  The attribute value.

Personally I like adding the dash. Makes things pretty clear where the description ends. (Aligning doesn't scale well). 

 @param {string} value -  The attribute value.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6527a5a0afdd
Renable and fix test_filter.js. r=mkmelin

Comment 15

[Patrick Cloke [:clokep]]

Thanks! I fixed those before pushing (plus had to fix a minor issue from another patch I had in my queue).