Open Bug 1727842 Opened 5 months ago Updated 2 months ago

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

Categories

(Core :: Security, defect)

Unspecified
Windows 10
defect

Tracking

()

Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- unaffected
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix

People

(Reporter: mccr8, Assigned: tjr, NeedInfo)

References

(Regression)

Details

(Keywords: crash, leave-open, regression, Whiteboard: [no-nag] )

Crash Data

Attachments

(1 file)

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.

Severity: -- → S2

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

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

Assignee: nobody → tom

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?

Flags: needinfo?(continuation)

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.

Flags: needinfo?(continuation)
See Also: → 1727930

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.

Flags: needinfo?(tom)

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

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.

Keywords: leave-open
Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd4da96d2f74
Do not crash if off the main thread. r=freddyb

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

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

Flags: needinfo?(tom)
Whiteboard: [no-nag]
You need to log in before you can comment on or make changes to this bug.