Closed Bug 1782501 Opened 3 years ago Closed 3 months ago

Crash in [@ nsMutationReceiver::ContentAppended] with parser-created about:blank and NoScript extension

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
145 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- fixed
firefox-esr140 --- fixed
firefox143 --- wontfix
firefox144 --- fixed
firefox145 --- fixed

People

(Reporter: gsvelto, Assigned: hsivonen)

References

Details

(Keywords: crash)

Crash Data

Attachments

(5 files, 1 obsolete file)

noscript-allowed-websites.webp
8.55 KB, image/webp
Details
Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug!
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug!
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr140+
Details | Review
Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug!
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr115+
Details | Review

Crash report: https://crash-stats.mozilla.org/report/index/23af13b2-1b8c-44c1-9c3d-ec9b00220801

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsMutationReceiver::ContentAppended dom/base/nsDOMMutationObserver.cpp:227
1 xul.dll static mozilla::dom::MutationObservers::NotifyContentAppended dom/base/MutationObservers.cpp:163
2 xul.dll nsContentSink::NotifyAppend dom/base/nsContentSink.cpp:576
3 xul.dll HTMLContentSink::OpenContainer dom/html/nsHTMLContentSink.cpp:786
4 xul.dll CNavDTD::BuildModel parser/htmlparser/CNavDTD.cpp:30
5 xul.dll nsParser::ResumeParse parser/htmlparser/nsParser.cpp:734
6 xul.dll nsParser::OnStopRequest parser/htmlparser/nsParser.cpp:1062
7 xul.dll nsDocumentOpenInfo::OnStopRequest uriloader/base/nsURILoader.cpp:216
8 xul.dll nsBaseChannel::OnStopRequest netwerk/base/nsBaseChannel.cpp:837
9 xul.dll nsInputStreamPump::OnStateStop netwerk/base/nsInputStreamPump.cpp:657

This does not appear like a new crash. It's a NULL pointer access and in particular it appears that aFirstNewContent was NULL. I'm filing under DOM: Forms because almost all crash URLs are either from banks, postal services or LinkedIn so I have a hunch that it might be related to filling some form.

Change to S3 - not a new crash, very low volume, not a security problem.

Severity: S2 → S3

Blocking on bug 1795643 because the signature changed here when we deployed support for inlined functions in crash reports.

Depends on: 1795643
Crash Signature: [@ nsMutationReceiver::ContentAppended] → [@ nsMutationReceiver::ContentAppended] [@ nsINode::GetParentNode | nsMutationReceiver::ContentAppended]

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

  • Top 10 desktop browser crashes on nightly

:farre, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(afarre)
Keywords: topcrash

60% of the crashes (i.e. 30 crashes) are from a single install, which seems to cause a false alarm of top-crash labelling.

Flags: needinfo?(afarre)

Based on the topcrash criteria, the crash signatures linked to this bug are not in the topcrash signatures anymore.

For more information, please visit BugBot documentation.

Keywords: topcrash

For me this is not related to forms filling. I get this crash intermittently when browsing someone's LinkedIn profile and either expanding their contact info, or activating a link which shows all their work / educational experience. For reference: I also have accessibility enabled since I am a screen reader user, not sure if this matters. If there is additional debugging info I could collect from the crashed tab next time this happens I'd be happy to help chasing this down.

Just FYI: this has happened once again today in the exact same scenario i.e. when browsing a LinkedIn profile.

I got this crash 3 times in a row when trying to view my profile viewers on LinkedIn. Can this please be looked into - with this crash parts of LinkedIn are intermittently unusable.

MIght this NoScript report be related?
https://github.com/hackademix/noscript/issues/385

In Firefox 143.0.1 for Linux (the official Mozilla build):

I cannot open this URL: https://www.gog.com/en/game/deus_ex_human_revolution_directors_cut

I've now submitted six crash reports.

Firefox keeps crashing.

PLEASE FIX.

I've checked a dozen of other random reports: all the affected people have NoScript installed.

Flags: needinfo?(echen)

Disabling NoScript fixes the issue but AddOns must not be able to crash the browser!

When opening the URL directly, nothing happens.

If you open the first game on the list when visiting this page:

https://www.gog.com/en/games?developers=eidos-montreal -> https://www.gog.com/en/game/deus_ex_human_revolution_directors_cut

the crash occurs 100% of the time.

Websites that are allowed/disallowed. All the other settings for NoScript are by default.

I was wrong. Now all GOG game pages crash the browser no matter how I open them.

Artem, could you share a crash report of a recent crash?
Load about:crashes and find a link there.

Flags: needinfo?(aros)

At least sINode::GetParentNode | nsMutationReceiver::ContentAppended crashes have nsParser on stack, and the crash is a null pointer crash.
hsivonen, can you think of how we could get a null pointer from nsHTMLContentSink?

Flags: needinfo?(echen) → needinfo?(hsivonen)
bp-9aca7ad7-0f30-43be-83aa-99bd80250921 	9/21/25, 2:14 PM 	View
bp-c99892f9-79ff-4222-966c-461a70250921 	9/21/25, 2:14 PM 	View
bp-eb30b06a-e550-4932-9b14-4d3610250921 	9/21/25, 2:14 PM 	View
bp-929e8666-23f7-4ed6-8c1d-2508a0250921 	9/21/25, 2:14 PM 	View
bp-3404c20d-ea66-4613-98b2-e68d50250921 	9/21/25, 2:14 PM 	View
bp-37a5fa0f-63f4-4900-86b3-42fd70250921 	9/21/25, 2:14 PM 	View
bp-a1c4eccc-1317-46bb-9960-bc28c0250921 	9/21/25, 2:14 PM 	View
bp-60d7b3b5-766d-4968-9d98-b63d30250921 	9/21/25, 2:14 PM 	View
bp-bb813b97-59b9-42f0-8665-d7b320250921 	9/21/25, 1:57 PM 	View
bp-f2596ca4-d29f-4c4b-8541-097cc0250921 	9/21/25, 1:57 PM 	View
bp-a679d04d-6d00-403b-a0a9-42d730250921 	9/21/25, 1:57 PM 	View
bp-6f836ef5-f22d-4cd5-9c03-1c39b0250921 	9/21/25, 1:57 PM 	View
bp-ac9456d5-4227-4f1f-bd4f-fecd10250921 	9/21/25, 1:57 PM 	View
bp-9f6d84a9-4f98-4e67-884e-d897c0250921 	9/21/25, 1:57 PM 	View
Flags: needinfo?(aros)

This code runs only for the parser-created about:blank. Since it's about having NoScript installed, the most likely explanation is that NoScript manages to modify the about:blank DOM as its being constructed.

This code is going away in bug 543435, but since we don't know how soon that's really going to land, I guess it makes sense to see if we can be more defensive against mutation by an extension here.

Flags: needinfo?(hsivonen)

Since we know we're always building the about:blank DOM, perhaps it's useless to notify at the point of opening body, and we could instead remove all the intermediate notification code and notify once at the end. smaug, would that work with the current notification architecture? (This is using deprecated notifications anyway.)

Flags: needinfo?(smaug)

(In reply to Artem S. Tashkinov from comment #12)

When opening the URL directly, nothing happens.

If you open the first game on the list when visiting this page:

https://www.gog.com/en/games?developers=eidos-montreal -> https://www.gog.com/en/game/deus_ex_human_revolution_directors_cut

the crash occurs 100% of the time.

I tried this with NoScript defaults in a debug build under rr on Linux, but it didn't crash.

I didn't verify this, but from working on bug 543435, I suspect NoScript manages to block the parser from the root insertion notification for about:blank.

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=154126

./mach try chooser no longer gives me the option to run mochitests, and chances are that there are problematic WebExtensions tests in the tests that are not offered.

It's pretty sad to modify this code at this point, but let's see if moving to new-style notifications and deferring root insertion after also head and body have been inserted works.

Flags: needinfo?(smaug)

An alternative to the draft patch here would be splitting the nsHtml5Parser part for non-initial navigation to about:blank out of bug 543435 and landing that part as a replacement for the old nsHTMLContentSink path as a fix for this bug.

Summary: Crash in [@ nsMutationReceiver::ContentAppended] → Crash in [@ nsMutationReceiver::ContentAppended] with parser-created about:blank and NoScript extension
Component: DOM: Forms → DOM: HTML Parser

(In reply to Henri Sivonen (:hsivonen) from comment #23)

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=154126

./mach try chooser no longer gives me the option to run mochitests, and chances are that there are problematic WebExtensions tests in the tests that are not offered.

See bug 1988962, fixed recently.

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

See bug 1988962, fixed recently.

Thanks.

I'm marking this bug as depending on bug 543435, since landing bug 543435 would fix this.

Depends on: sync-about-blank

I don't know what "deprecated notifications" mean

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #29)

I don't know what "deprecated notifications" mean

The code does notifications the old way (trying to avoid eager notifications, trying to keep track of what it has already notified about, and deferring notifications). However, the thing that has "Deprecated" in the name wasn't actually the notification part but GetChildAt_Deprecated.

I started making the simple null check change that was discussed yesterday, and the resulting null situation looked broken enough that I went back to my earlier patch and tried moving the root insertion notification runnable in that patch to see if that would make things green:
https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=154681

(In reply to Henri Sivonen (:hsivonen) from comment #32)

I started making the simple null check change that was discussed yesterday, and the resulting null situation looked broken enough that I went back to my earlier patch and tried moving the root insertion notification runnable in that patch to see if that would make things green:
https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=154681

Clearly, the simple null check is the way to go even if it leaves the code in state that looks concerning.

Attachment #9514629 - Attachment is obsolete: true
Assignee: nobody → hsivonen
Attachment #9517075 - Attachment description: WIP: Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug! → Bug 1782501 - Be defensive against flush counts getting out of sync for parser-created about:blank. r?smaug!
Status: NEW → ASSIGNED
Pushed by hsivonen@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/035aa3a44841 https://hg.mozilla.org/integration/autoland/rev/5986eee23bc7 Be defensive against flush counts getting out of sync for parser-created about:blank. r=smaug

https://hg.mozilla.org/mozilla-central/rev/5986eee23bc7

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
status-firefox145: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 145 Branch

The patch landed in nightly and beta is affected.
:hsivonen, is this bug important enough to require an uplift?

For more information, please visit BugBot documentation.

Flags: needinfo?(hsivonen)

Artem, does the latest Nightly resolve the crash for you?

Flags: needinfo?(hsivonen) → needinfo?(aros)

(In reply to Henri Sivonen (:hsivonen) from comment #39)

Artem, does the latest Nightly resolve the crash for you?

I cannot reproduce it on the latest Nightly but then it didn't always occur on the release version either ... so there's a chance it's been fixed but I cannot be 100% sure.

Built from https://hg.mozilla.org/mozilla-central/rev/75a595a9d65d529c86a28bc528c86caf24891d85

Flags: needinfo?(aros)

firefox-beta Uplift Approval Request

  • User impact if declined: The patch is believed to address crashes with the well-known NoScript extension.
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing:
  • Risk associated with taking this patch: low
  • Explanation of risk level: The patch avoids calling code when an inconsistent state is detected. The change is expected not to change anything when the state is not inconsistent.
  • String changes made/needed: None.
  • Is Android affected?: yes
Attachment #9517518 - Flags: approval-mozilla-beta?

(In reply to Artem S. Tashkinov from comment #40)

(In reply to Henri Sivonen (:hsivonen) from comment #39)

Artem, does the latest Nightly resolve the crash for you?

I cannot reproduce it on the latest Nightly but then it didn't always occur on the release version either ... so there's a chance it's been fixed but I cannot be 100% sure.

Built from https://hg.mozilla.org/mozilla-central/rev/75a595a9d65d529c86a28bc528c86caf24891d85

Thanks!

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #38)

The patch landed in nightly and beta is affected.
:hsivonen, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.

Requested beta uplift on the assumption that this actually fixes a crash with a well-known extension.

Attachment #9517518 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Please nominate this for ESR140 also.

Flags: needinfo?(hsivonen)

firefox-esr140 Uplift Approval Request

  • User impact if declined: The patch is believed to address crashes with the well-known NoScript extension.
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing:
  • Risk associated with taking this patch: low
  • Explanation of risk level: The patch avoids calling code when an inconsistent state is detected. The change is expected not to change anything when the state is not inconsistent.
  • String changes made/needed: None.
  • Is Android affected?: yes
Attachment #9517684 - Flags: approval-mozilla-esr140?

firefox-esr115 Uplift Approval Request

  • User impact if declined: The patch is believed to address crashes with the well-known NoScript extension. Requesting uplift even to ESR 115 on the assumption that ESR 115 users might run NoScript as defense in depth in an old browser.
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing:
  • Risk associated with taking this patch: low
  • Explanation of risk level: The patch avoids calling code when an inconsistent state is detected. The change is expected not to change anything when the state is not inconsistent.
  • String changes made/needed: None.
  • Is Android affected?: yes
Attachment #9517685 - Flags: approval-mozilla-esr115?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)

Please nominate this for ESR140 also.

Requested uplift to ESR 140 and even ESR 115 on the assumption that users of old ESR might be running NoScript as defense in depth. (The code being patched here is ancient from 1999. We're going to remove the code in bug 543435 after which ESR and Nightly/release will diverge on this point and we'll no longer have Nightly/release crash visibility into problems that might affect ESR, too.)

Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #50)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)

Please nominate this for ESR140 also.

Requested uplift to ESR 140 and even ESR 115 on the assumption that users of old ESR might be running NoScript as defense in depth.

Not just that: the Tor Browser* and the Mullvad Browser are based on ESR and ship NoScript by default.

* We still maintain an ESR115-based "Legacy" version, too.

Attachment #9517685 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Attachment #9517684 - Flags: approval-mozilla-esr140? → approval-mozilla-esr140+
QA Whiteboard: [qa-triage-done-c145/b144]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: