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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Vincent Hilla [:vhilla]]

Crash report: https://crash-stats.mozilla.org/report/index/e3799b4d-4405-4f7c-ac7c-5dc1c0260713

This is coming from a different check than bug 2048851. That bug was regressed by bug 2045635, which is only in Nightly so far, it's fixed and the crash rate in nightly goes down. But we also see an increase in crashes on beta.

Comment 1

[Vincent Hilla [:vhilla]]

The crashes on beta all go through ipc/ipdl/PBrowserBridgeParent.cpp:471, which is PBrowserBridge::Msg_LoadURL__ID. The three ESR ones are PNecko::Msg_PDocumentChannelConstructor__ID.

Comment 3

[Vincent Hilla [:vhilla]]

I hope the crash volume won't increase too much. I don't know how these crashes occur, a reproducible example would be helpful. If the rate increases, we could early return in release builds before SendLoadURL to at least avoid content crashes. Maybe someone can have a look at protected data, i.e. what pages are crashing?

Comment 4

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content. r=#dom-core

Comment 5

[Vincent Hilla [:vhilla]]

https://treeherder.mozilla.org/jobs?repo=try&revision=0d9e2f99fe671561174db92d011ef4f81f5cc4d3

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e8e1a06347f1
https://hg.mozilla.org/integration/autoland/rev/6d74d59dc69b
Reject cross process javascript URIs in content. r=dom-core-reviewers,jjaschke

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/61403 for changes under testing/web-platform/tests

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/6d74d59dc69b

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 11

[Dianna Smith [:diannaS]]

:vhilla did you want to uplift this to 154 beta? If so, please add uplift requests

Comment 12

[Vincent Hilla [:vhilla]]

STR

• Go to https://example.com
• In the console, enter document.body.append(Object.assign(document.createElement("iframe"), { src: "https://example.org" }));
• In the console, enter document.querySelector("iframe").src = "javascript:0;"

Comment 13

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content.

Original Revision: https://phabricator.services.mozilla.com/D312706

Comment 14

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: parent-process crash
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See comment 12
• Risk associated with taking this patch: low
• Explanation of risk level: Reject cross origin javascript URIs through iframe.src in content, as we already do for window.open and location. The parent would reject these anyway.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 15

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: parent-process crash
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See comment 12
• Risk associated with taking this patch: low
• Explanation of risk level: Reject cross origin javascript URIs through iframe.src in content, as we already do for window.open and location. The parent would reject these anyway.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 16

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content.

Original Revision: https://phabricator.services.mozilla.com/D312706

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content.

Original Revision: https://phabricator.services.mozilla.com/D312706

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: parent-process crash
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See comment 12
• Risk associated with taking this patch: low
• Explanation of risk level: Reject cross origin javascript URIs through iframe.src in content, as we already do for window.open and location. The parent would reject these anyway.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 19

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content.

Original Revision: https://phabricator.services.mozilla.com/D312706

Comment 20

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: parent-process crash
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See comment 12
• Risk associated with taking this patch: low
• Explanation of risk level: Reject cross origin javascript URIs through iframe.src in content, as we already do for window.open and location. The parent would reject these anyway.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 21

[Vincent Hilla [:vhilla]]

Attachment: Bug 2054485 - Reject cross process javascript URIs in content.

Original Revision: https://phabricator.services.mozilla.com/D312706

Comment 22

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: parent-process crash
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See comment 12
• Risk associated with taking this patch: low
• Explanation of risk level: Reject cross origin javascript URIs through iframe.src in content, as we already do for window.open and location. The parent would reject these anyway.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 23

[Vincent Hilla [:vhilla]]

To clarify, html spec step 4 rejects cross origin javascript URIs, Chrome and Firefox do too. Web pages should not have a reason to trigger this bug, though a buggy page might do so and cause parent process crashes.

Comment 24

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/35535dd5dca7
https://hg.mozilla.org/releases/mozilla-beta/rev/323774e7f0b6

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/ce742de1a726

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/bea9ff2dd78d

Comment 27

[Wayne Mery (:wsmwk)]

#2 crash for Thunderbird release 153.0 (non-ESR). All about opening PDFs

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/e628c8a6ce68

Comment 29

[Vincent Hilla [:vhilla]]

The few Thunderbird failures I looked through are a different error. The sudden increase in frequency is surprising to me, I don't immediately see what change could've caused that.

nsDocShellLoadState URI changed while in content process

Do we have a bug that tracks the Thunderbird crashes? This bug is likely about something else and my change probably won't fix it.

Comment 30

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/cf9325f0285a
https://hg.mozilla.org/releases/mozilla-release/rev/3b7c26cb9a52

Comment 31

[Wayne Mery (:wsmwk)]

(In reply to Vincent Hilla [:vhilla] from comment #29)

The few Thunderbird failures I looked through are a different error. The sudden increase in frequency is surprising to me, I don't immediately see what change could've caused that.

nsDocShellLoadState URI changed while in content process

Do we have a bug that tracks the Thunderbird crashes? This bug is likely about something else and my change probably won't fix it.

Filed bug 2057980