1727842 - Crash in [@ mozilla::ipc::MessageChannel::Send | mozilla::dom::PContentParent::SendPreferenceUpdate]

Status: RESOLVED WONTFIX

(Core :: Security, defect)

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/3c1d9207-7b01-416d-bc18-10d100210826

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(mWorkerThread && mWorkerThread->IsOnCurrentThread()) (not on worker thread!)

Top 10 frames of crashing thread:

0 xul.dll mozilla::ipc::MessageChannel::Send ipc/glue/MessageChannel.cpp:885
1 xul.dll mozilla::dom::PContentParent::SendPreferenceUpdate ipc/ipdl/PContentParent.cpp:2585
2 xul.dll mozilla::dom::ContentParent::Observe dom/ipc/ContentParent.cpp:3539
3 xul.dll static nsPrefBranch::NotifyObserver modules/libpref/Preferences.cpp:2688
4 xul.dll NotifyCallbacks modules/libpref/Preferences.cpp:1684
5 xul.dll pref_SetPref modules/libpref/Preferences.cpp:1642
6 xul.dll static mozilla::Preferences::SetInt modules/libpref/Preferences.cpp:4888
7 xul.dll PossiblyCrash dom/security/nsContentSecurityUtils.cpp:498
8 xul.dll static nsContentSecurityUtils::ValidateScriptFilename dom/security/nsContentSecurityUtils.cpp:1238
9 xul.dll static js::ScriptSourceObject::initFromOptions js/src/vm/JSScript.cpp:1764

This is crashing in code that was added in bug 1723204. It looks like the issue is that this code is trying to set a pref using IPC, but it is being triggered by some JS that is being run off the main thread for ProxyAutoConfig.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1723204

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I disabled the code that would cause this, so it shouldn't recur, but I should fix it.

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Actually, I don't think I understand this after all.

1. The current thread is not the main thread.
2. I'm trying to set a pref, which has to go over "IPC" (but it's really intra-process because we're already in the parent) because although we're in the the parent we're not on main thread and I guess pref setting needs to happen on the main thread?
3. The pref-setting MessageChannel is bound (mThread) to the main thread? But that doesn't make sense to me....

Could you unpack the issue for me a little more?

Comment 4

[Andrew McCreight [:mccr8]]

No, I don't think your point 2 is correct. It looks like ContentParent is watching for preferences getting changed, then sending a message to its child process to tell it that the pref changed. ContentParent is associated with the main thread and we're on some other thread so it crashes.

Also, it looks like pref_SetPref asserts that we're on the main thread, so things have already gone off the rails before we try to send the IPC message.

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

It seems like

1. I need to exempt the PAC script from filename validation. That makes sense
2. I need to check if we're Main Thread in PossiblyCrash, and if not, return early, just like I do if we're not Parent Process.

I'll do that when I get back from PTO.

Comment 6

[Andrew McCreight [:mccr8]]

Yeah, besides this PAC code, we also run JS on worker threads, so it'd be good to handle that.

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Hm. This is confusing. PAC Scripts already disable filename validation. I do not see how this could have happened... I'll have to try to test it locally.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1727842: Do not crash if off the main thread. r?freddyb

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd4da96d2f74
Do not crash if off the main thread. r=freddyb

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bd4da96d2f74

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Tom, should we keep this as leave-open?
Does the patch help?

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I need to test the PAC stuff locally and potentially add a follow up patch

Comment 13

[Dianna Smith [:diannaS]]

:tjr, do you have time to confirm if this is already addressed? It seems like the patch helped, but if not ...can we evaluate if this is still an S2?

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

It's not an S2; so it can be reclassified; but it shouldn't be closed.

Comment 15

[Marco Castelluccio [:marco]]

Unsetting severity so the triage owner can reclassify.

Comment 16

[Frederik Braun [:freddy]]

Tom: If I'm not mistaken there are no recent crashes. Can we close this?

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

This is something I should still dig into that is relevant from JS Load Telemetry

Comment 18

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'm still curious about this, but I will need new data to see if this reproduces.