Closed Bug 2048851 Opened 28 days ago Closed 17 days ago

Crash in [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::ipc::IProtocol::FatalError | nsDocShellLoadState::nsDocShellLoadState]

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Platform:
Unspecified
Windows 11
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
154 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- fixed
firefox-esr140 --- fixed
firefox152 --- unaffected
firefox153 --- fixed
firefox154 + fixed

People

(Reporter: gsvelto, Assigned: vhilla)

References

(Regression)

Details

(4 keywords)

Crash Data

Attachments

(4 files)

Bug 2048851 - Clear principalToInherit in LoadPageAsViewSource. r=#dom-core
48 bytes, text/x-phabricator-request
Details | Review
Bug 2048851 - Clear principalToInherit in LoadPageAsViewSource.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 2048851 - Clear principalToInherit in LoadPageAsViewSource.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr140+
Details | Review
Bug 2048851 - Clear principalToInherit in LoadPageAsViewSource.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr115+
Details | Review

Crash report: https://crash-stats.mozilla.org/report/index/25ab4482-df71-4139-b6e2-15e660260618

MOZ_CRASH Reason:

MOZ_CRASH(IPC FatalError in the parent process!)

Top 10 frames:

0  xul.dll  mozilla::ipc::FatalError(char const*, bool)  ipc/glue/ProtocolUtils.cpp:205
1  xul.dll  mozilla::ipc::IProtocol::HandleFatalError(char const*)  ipc/glue/ProtocolUtils.cpp:371
2  xul.dll  mozilla::ipc::IProtocol::FatalError(char const* const)  ipc/glue/ProtocolUtils.cpp:362
2  xul.dll  nsDocShellLoadState::nsDocShellLoadState(mozilla::dom::DocShellLoadStateInit ...  docshell/base/nsDocShellLoadState.cpp:0
2  xul.dll  IPC::ParamTraits<nsDocShellLoadState*>::Read(IPC::MessageReader*, RefPtr<nsDo...  dom/ipc/DocShellMessageUtils.cpp:36
2  xul.dll  IPC::ParamTraitsMozilla<RefPtr<nsDocShellLoadState> >::Read(IPC::MessageReade...  ipc/chromium/src/chrome/common/ipc_message_utils.h:1447
2  xul.dll  IPC::ReadParam(IPC::MessageReader*)  ipc/chromium/src/chrome/common/ipc_message_utils.h:522
2  xul.dll  IPC::ParamTraitsMozilla<mozilla::NotNull<RefPtr<nsDocShellLoadState> > >::Rea...  ipc/chromium/src/chrome/common/ipc_message_utils.h:1474
3  xul.dll  IPC::ReadParam(IPC::MessageReader*)  ipc/chromium/src/chrome/common/ipc_message_utils.h:519
3  xul.dll  IPC::ParamTraits<mozilla::net::DocumentChannelCreationArgs>::Read(IPC::Messag...  ipc/ipdl/NeckoChannelParams.cpp:5098

New crash triggered by the changes in bug 2045635. I'm not marking that bug as a regression given the crash is intentional but feel free to change that if you feel it's appropriate.

macOS-specific signature

Crash Signature: [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::ipc::IProtocol::FatalError | nsDocShellLoadState::nsDocShellLoadState] → [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::ipc::IProtocol::FatalError | nsDocShellLoadState::nsDocShellLoadState] [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | nsDocShellLoadState::nsDoc…
Flags: needinfo?(vhilla)

The instances I looked at all went through DocumentChannelCreationArgs and have nsDocShellLoadState with invalid principalToInherit. I didn't find any commonality between addons.

I don't know in which circumstances these crashes occur. Perhaps we can crash earlier around SendPDocumentChannelConstructor. The resulting stack in the content process might be useful and we won't crash the parent anymore.

Flags: needinfo?(vhilla)
Depends on: 2050899

STR

  • Go to example.com
  • Navigate to example.org in a way that inherits from the current document
    • E.g. through a link click, form submission, or window.location but not via the URL bar.
  • view page source

When navigating to example.org, the principal to inherit on load state is example.com. We create a session history entry from the load state, which also has principal to inherit being example.com. When we then view page source, LoadPageAsViewSource calls FillLoadStateFromCurrentEntry and copies over the example.com principal to inherit despite doing an example.org view source.

I think the meaning of principal to inherit is a bit blurry, but something like the principal we would be inheriting if we want to. We only want to do so for special documents like about:blank, definitely not for cross origin navigations. Perhaps we could just clear it when we decide against using it, but that seems risky. I guess it also makes sense to store the example.com principal to inherit in session history for example.org in case we restore this entry.

But LoadPageAsViewSource originates in example.org and should therefore not have example.com as principal to inherit. I wonder if we have the same issue in other places where we use FillLoadStateFromCurrentEntry.

LLM additions: The session history entry stores the full load state needed to recreate the document, including the principalToInherit from the original navigation. This is correct for session history restores (back/forward). However, LoadPageAsViewSource uses FillLoadStateFromCurrentEntry to build a new load state originating from the current page rather than recreating the load to it. In that case, the stored principalToInherit (from the prior document that initiated the navigation) is incorrect for the new load. The fix clears principalToInherit in LoadPageAsViewSource after filling from the entry, since view-source derives its own principal. FillLoadStateFromCurrentEntry has no other callers today. Other callers are not impacted.

Assignee: nobody → vhilla
Status: NEW → ASSIGNED
Regressed by: 2045635
Pushed by vhilla@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/fdf8fbfdd850 https://hg.mozilla.org/integration/autoland/rev/b5600c69a8fb Clear principalToInherit in LoadPageAsViewSource. r=dom-core-reviewers,smaug

The bug is linked to a topcrash signature, which matches the following criterion:

  • Top 10 desktop browser crashes on nightly

For more information, please visit BugBot documentation.

Keywords: topcrash

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

Status: ASSIGNED → RESOLVED
Closed: 17 days ago
status-firefox154: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 154 Branch
See Also: → 2054139

firefox-beta Uplift Approval Request

  • User impact if declined/Reason for urgency: Fix bug 2054139
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing: This is covered by toolkit/components/viewsource/test/browser/browser_bug2048851.js and browser/components/tabbrowser/test/browser/tabs/browser_viewsource_of_data_URI_in_file_process.js.
    See comment 4 for steps regarding the first test. I believe the second one cannot be reproduced manually because we block pages from navigating to top level data URIs.
  • Risk associated with taking this patch: low
  • Explanation of risk level: This has been in Nightly for two weeks and only impacts view-source for URIs that don't provide their own security context, e.g. data and srcdoc.
  • String changes made/needed?: No
  • Is Android affected?: yes
Attachment #9607066 - Flags: approval-mozilla-beta?

firefox-esr140 Uplift Approval Request

  • User impact if declined/Reason for urgency: Fix bug 2054139
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing: This is covered by toolkit/components/viewsource/test/browser/browser_bug2048851.js and browser/components/tabbrowser/test/browser/tabs/browser_viewsource_of_data_URI_in_file_process.js.
    See comment 4 for steps regarding the first test. I believe the second one cannot be reproduced manually because we block pages from navigating to top level data URIs.
  • Risk associated with taking this patch: low
  • Explanation of risk level: This has been in Nightly for two weeks and only impacts view-source for URIs that don't provide their own security context, e.g. data and srcdoc.
  • String changes made/needed?: No
  • Is Android affected?: yes
Attachment #9607067 - Flags: approval-mozilla-esr140?
Attachment #9607066 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9607067 - Flags: approval-mozilla-esr140? → approval-mozilla-esr140+

We also need an uplift to esr115, thanks.

Flags: needinfo?(vhilla)

esr115 failed via Lando, I'll rebase manually.

Flags: needinfo?(vhilla)
Attachment #9607073 - Flags: approval-mozilla-esr115?

firefox-esr115 Uplift Approval Request

  • User impact if declined/Reason for urgency: Fix bug 2054139
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing: This is covered by toolkit/components/viewsource/test/browser/browser_bug2048851.js and browser/components/tabbrowser/test/browser/tabs/browser_viewsource_of_data_URI_in_file_process.js.
    See comment 4 for steps regarding the first test. I believe the second one cannot be reproduced manually because we block pages from navigating to top level data URIs.
  • Risk associated with taking this patch: medium
  • Explanation of risk level: This has been in Nightly for two weeks and only impacts view-source for URIs that don't provide their own security context, e.g. data and srcdoc.
    Though esr115 is old. I looked through the important bits for loading view-source and they're similar. But I'm unable to push esr115 to try.
  • String changes made/needed?: No
  • Is Android affected?: yes
Attachment #9607073 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Regressions: 2054487
QA Whiteboard: [qa-triage-done-c154/b153]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: