1978727 - Hit MOZ_CRASH(DocumentLoadListener::OpenDocument: Unexpected load flags: 290005 vs. 290001 (differing 4 vs. 0)) at /builds/worker/checkouts/gecko/netwerk/ipc/DocumentLoadListener.cpp:1037

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing 20250525-a0c6745660c1 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Hit MOZ_CRASH(DocumentLoadListener::OpenDocument: Unexpected load flags: 290005 vs. 290001 (differing 4 vs. 0)) at /builds/worker/checkouts/gecko/netwerk/ipc/DocumentLoadListener.cpp:1037

#0 0x7a3c82063402 in MOZ_CrashSequence /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:248:3
#1 0x7a3c82063402 in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:381:3
#2 0x7a3c82063402 in mozilla::net::DocumentLoadListener::OpenDocument(nsDocShellLoadState*, unsigned int, unsigned int, mozilla::Maybe<unsigned long> const&, mozilla::TimeStamp const&, nsDOMNavigationTiming*, mozilla::Maybe<mozilla::dom::ClientInfo>&&, bool, mozilla::Maybe<bool>, mozilla::dom::ContentParent*, nsresult*) /builds/worker/checkouts/gecko/netwerk/ipc/DocumentLoadListener.cpp:1061:7
#3 0x7a3c82062165 in mozilla::net::DocumentChannelParent::Init(mozilla::dom::CanonicalBrowsingContext*, mozilla::net::DocumentChannelCreationArgs const&) /builds/worker/checkouts/gecko/netwerk/ipc/DocumentChannelParent.cpp:69:40
#4 0x7a3c82080d3a in mozilla::net::NeckoParent::RecvPDocumentChannelConstructor(mozilla::net::PDocumentChannelParent*, mozilla::dom::MaybeDiscarded<mozilla::dom::BrowsingContext> const&, mozilla::net::DocumentChannelCreationArgs const&) /builds/worker/checkouts/gecko/netwerk/ipc/NeckoParent.cpp:270:11
#5 0x7a3c8210dae4 in mozilla::net::PNeckoParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PNeckoParent.cpp:1871:79
#6 0x7a3c86a7f5c8 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentParent.cpp:6439:32
#7 0x7a3c8229f2ce in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1795:25
#8 0x7a3c8229c850 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, std::unique_ptr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1721:9
#9 0x7a3c8229d257 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1512:3
#10 0x7a3c8229e239 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1612:14
#11 0x7a3c816cfee7 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:703:16
#12 0x7a3c816c8fee in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:1310:20
#13 0x7a3c816c7d27 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:1133:15
#14 0x7a3c816c81a5 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:639:36
#15 0x7a3c816d6f26 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:333:37
#16 0x7a3c816d6f26 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#17 0x7a3c816e8b23 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1159:16
#18 0x7a3c816ef24f in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#19 0x7a3c822a4a67 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#20 0x7a3c822005b1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#21 0x7a3c822005b1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#22 0x7a3c8707af48 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#23 0x7a3c8713f354 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:471:33
#24 0x7a3c87f3cc84 in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:291:30
#25 0x7a3c8805ba4e in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5893:22
#26 0x7a3c8805ced7 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:6138:8
#27 0x7a3c8805d9f8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:6211:21
#28 0x648ff8f8ef66 in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:232:22
#29 0x648ff8f8ef66 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:464:16
#30 0x7a3c96a72d8f in __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#31 0x7a3c96a72e3f in __libc_start_main ./csu/../csu/libc-start.c:392:3
#32 0x648ff8f62588 in _start ??:0:0

Comment 1

[Tyson Smith [:tsmith]]

This has also been reported by live site testing.

Comment 2

[Mayank Bansal]

Bisection:
Bug 1921972 - Allow to propagate loadgroup flags to parent process. r=nika,necko-reviewers,valentin

As per discussion. Test is in the other patch, assuming this is green
I'll incorporate it, but gotta go for the day.

Differential Revision: https://phabricator.services.mozilla.com/D224824

Comment 3

[BugBot [:suhaib / :marco]]

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

:emilio, since you are the author of the regressor, bug 1921972, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(Please no rush responding, I don't think this is super prioritary as it's a DIAGNOSTIC_ASSERT, doesn't crash on release)

Ok, so the differing flag is LOAD_DOCUMENT_NEEDS_COOKIE, and we're not getting it set from the child, but are getting it set from the parent.

So there's a mismatch between what DocumentLoadListener says the sandbox flags should be vs what they really are. But afaict they come from the same place (BrowsingContext::GetSandboxFlags):

• https://searchfox.org/mozilla-central/rev/1838f847aa3bf909c3d34a94a8f0cd7e37fca086/docshell/base/nsDocShell.cpp#10822,10849-10852

I think this is a pre-existing race condition that my patch exposes (because it added the assert). The test-case does:

  frame.src = "...";
  setTimeout(() => {
    frame.srcdoc = "<div></div>" // nsDocShell::DoURILoad happens here. No sandbox flags.
    frame.setAttribute("sandbox", "allow-top-navigation") // BrowsingContext sandbox flags are updated in both the child and parent processes.
  }, 750);

So by the time DocumentLoadListener::Open runs, the sandbox flags are different from the time when the load started.

Andreas, Nika: Am I missing something? Does that diagnostic make sense to you?

If so, as for the fix: we could allow to set this bit from the child. If this is only about access to document.cookie it might be fine?

diff --git a/netwerk/base/nsLoadGroup.h b/netwerk/base/nsLoadGroup.h
index 18d6066e7fa89..ca308899508c0 100644
--- a/netwerk/base/nsLoadGroup.h
+++ b/netwerk/base/nsLoadGroup.h
@@ -70,8 +70,9 @@ class nsLoadGroup : public nsILoadGroup,
    * thus nothing security-critical should be allowed here.
    */
   static constexpr nsLoadFlags kInheritedLoadFlags =
-      LOAD_BACKGROUND | LOAD_BYPASS_CACHE | LOAD_FROM_CACHE | VALIDATE_ALWAYS |
-      VALIDATE_ONCE_PER_SESSION | VALIDATE_NEVER;
+      LOAD_BACKGROUND | LOAD_DOCUMENT_NEEDS_COOKIE | LOAD_BYPASS_CACHE |
+      LOAD_FROM_CACHE | VALIDATE_ALWAYS | VALIDATE_ONCE_PER_SESSION |
+      VALIDATE_NEVER;

  protected:
   virtual ~nsLoadGroup();

Alternatively, we could not fatally crash, I think that'd reject some loads incorrectly, which is what happens on release now. Given this is hit on the wild I'm not sure it's fine...

Any other potential fix I might've overlooked? I'll submit the fix I have tentatively, for now.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1978727 - Allow LOAD_DOCUMENT_NEEDS_COOKIE to differ in DocumentLoadListener::Open. r=nika,farre

As the sandbox flags may have changed by the time we compute the flags
on the parent.

Comment 6

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20250722162531-73304b4f70e7.
The bug appears to have been introduced in the following build range:

Start: 04e9738d508bf5eb0acf306e2671da47bab0ae7c (20241008210917)
End: 2a8c1b7d39e3a513cfb32fe857592d0c87fd945a (20241008230122)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=04e9738d508bf5eb0acf306e2671da47bab0ae7c&tochange=2a8c1b7d39e3a513cfb32fe857592d0c87fd945a

Comment 7

[Jason Kratzer [:jkratzer]]

Bugmon was unable to record a pernosco session for this bug.

Comment 8

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 9

[Bugmon [:jkratzer for issues]]

Testcase crashes using the initial build (mozilla-central 20250525085057-a0c6745660c1) but not with tip (mozilla-central 20251115092723-353e1356c088.)

Unable to bisect testcase (End build crashes!):

Start: a0c6745660c11fd82aee0812c5a5852c924468f6 (20250525085057)
End: 353e1356c0885bd6ad985bd00769e0e6483e36ef (20251115092723)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False, searchfox=False, afl=False)

Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Bugmon [:jkratzer for issues] from comment #9)

Testcase crashes using the initial build (mozilla-central 20250525085057-a0c6745660c1) but not with tip (mozilla-central 20251115092723-353e1356c088.)

Unable to bisect testcase (End build crashes!):

That reads kind of contradicting itself: "not with tip" vs. "End build crashes!" ?

Comment 11

[Jason Kratzer [:jkratzer]]

(In reply to Jens Stutte [:jstutte] from comment #10)

(In reply to Bugmon [:jkratzer for issues] from comment #9)

Testcase crashes using the initial build (mozilla-central 20250525085057-a0c6745660c1) but not with tip (mozilla-central 20251115092723-353e1356c088.)

Unable to bisect testcase (End build crashes!):

That reads kind of contradicting itself: "not with tip" vs. "End build crashes!" ?

It very much is contradictory. Looks like bugmon thought the bug was fixed but then bisection attempts triggered a crash on the end build. I'm looking into this.

Comment 12

[Bugmon [:jkratzer for issues]]

Testcase crashes using the initial build (mozilla-central 20250605212508-8cf3b09b71da) but not with tip (mozilla-central 20260605132559-470c1ac19c4c.)

Unable to bisect testcase (End build crashes!):

Start: 8cf3b09b71da83703e0b0285ff6d81b01ddc3c1d (20250605212508)
End: 470c1ac19c4c35d268348b5c9aba9f5edc1b4430 (20260605132559)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False, searchfox=False, afl=False)

Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 13

[Andreas Farre [:farre]]

Emilio's patch is set to needs revision, unsetting my needinfo waiting for that.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Sorry, this fell off my radar. I think it was ticked as waiting for changes while waiting for a pernosco session for nika to look at. There's one in comment 8, I've kicked it so it starts loading now.

Jason, any chance to get a more recent pernosco trace? My vague memory of this was that this approach was correct but maybe we can come up with something less footgunny?

Comment 15

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/utm7Yg-3i19ESWVIHtrcOw/index.html

(leaving ni? regarding "something less footgunny")

Comment 16

[Jason Kratzer [:jkratzer]]

Currently, the only way to trigger bugmon to fetch a pernosco trace on a newer build is to use [bugmon:origRev=<hash_of_recent_build>] and then add the pernosco-wanted keyword. I'll open an issue to add a more explicit whiteboard string.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

I got asked to look at this again because we got some more reports of this in the wild. I think the analysis in comment 4 and the questions are still relevant, but I tweaked the patch to be more targeted and just allow the flag to differ in DocumentLoadListener. I think that should address Nika's concerns. Note that bug 2028758 is also very related here. Nika, are you ok with the new version of the patch?

But I'm a bit confused about this, how are we getting those bits otherwise? Via mDefaultFlags?

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1978727 - Centralize loadgroup flag inheritance checks.

Untested...

Comment 19

[Emilio Cobos Álvarez [:emilio]]

(refreshing the ni? given the info above)

Comment 20

[Nika Layzell [:nika] (ni? for response)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)

I got asked to look at this again because we got some more reports of this in the wild. I think the analysis in comment 4 and the questions are still relevant, but I tweaked the patch to be more targeted and just allow the flag to differ in DocumentLoadListener. I think that should address Nika's concerns. Note that bug 2028758 is also very related here. Nika, are you ok with the new version of the patch?

I've left a r+ on the patch, though perhaps there is more that I'd want to improve here. Theoretically based on the spec, we shouldn't be snapshotting the sandbox flags synchronously unless the target window is in the same agent I think, but I would hope that sites aren't intentionally relying on the racy behaviour according to the spec.

But I'm a bit confused about this, how are we getting those bits otherwise? Via mDefaultFlags?

These flags were being inherited by mLoadFlags into subresource requests, when they should only be set for specific navigations which need those flags. Specifically, LOAD_HTML_OBJECT_DATA is set for object loads here: https://searchfox.org/firefox-main/rev/b478a70dbe9b20189bb57f12c05bc0d6a8f323bd/dom/base/nsObjectLoadingContent.cpp#1458, and LOAD_DOCUMENT_NEEDS_COOKIE is set for unsandboxed document loads here: https://searchfox.org/firefox-main/rev/b478a70dbe9b20189bb57f12c05bc0d6a8f323bd/docshell/base/nsDocShellLoadState.cpp#1461.

We don't want to inherit these flags into child loads, as they are specifically only relevant to the load which originated them.

I think my description on https://bugzilla.mozilla.org/show_bug.cgi?id=2028758#c16 has more context.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Right, I see. Those were being set because this code is pulling the flags directly from the loadgroup, instead of using the MergeLoadFlags stuff or what not, right?

Wouldn't it be better to just prevent those flags from showing up in mLoadFlags to begin with much like comment 18 attempts (note that has some orange, would need to dig into it)?

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e875038c754b
https://hg.mozilla.org/integration/autoland/rev/560d086f7789
Allow LOAD_DOCUMENT_NEEDS_COOKIE to differ in DocumentLoadListener::Open. r=nika,necko-reviewers,jesup

Comment 23

[Emilio Cobos Álvarez [:emilio]]

i guess that's sorta what's happening already. My patch applied the same list to the default flags which is what causes the orange...

Comment 24

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/560d086f7789

Comment 25

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

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

Comment 26

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

Upstream PR merged by moz-wptsync-bot

Comment 27

[Nika Layzell [:nika] (ni? for response)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)

Wouldn't it be better to just prevent those flags from showing up in mLoadFlags to begin with much like comment 18 attempts (note that has some orange, would need to dig into it)?

Yeah, I'm not opposed to preventing these flags from appearing in the load group in the first place, but I haven't fully thought through the consequences there, so there might be something else which could lead to conflicts there.

Comment 28

[Phabricator Automation]

Comment on attachment 9649731 [details]
WIP: Bug 1978727 - Centralize loadgroup flag inheritance checks.

Revision D329635 was moved to bug 2076541. Setting attachment 9649731 [details] to obsolete.