2004647 - Crash in [@ nsFrameLoader::ReallyStartLoadingInternal]

Status: RESOLVED DUPLICATE of bug 2007774

(Core :: DOM: Navigation, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/0e566a39-ee4e-49a1-aa8a-769eb0251205

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  nsCOMPtr<nsIPrincipal>::get() const  xpcom/base/nsCOMPtr.h:750
0  xul.dll  nsCOMPtr<nsIPrincipal>::operator nsIPrincipal*() const  xpcom/base/nsCOMPtr.h:758
0  xul.dll  nsFrameLoader::ReallyStartLoadingInternal()  dom/base/nsFrameLoader.cpp:761
0  xul.dll  nsFrameLoader::ReallyStartLoading()  dom/base/nsFrameLoader.cpp:639
0  xul.dll  mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders()  dom/base/Document.cpp:10079
1  xul.dll  mozilla::detail::RunnableMethodArguments<>::apply<nsObserverService, void (ns...  xpcom/threads/nsThreadUtils.h:1083
1  xul.dll  std::invoke(mozilla::detail::RunnableMethodArguments<>::apply<nsObserverServi...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/type_traits:1729
1  xul.dll  std::_Apply_impl(mozilla::detail::RunnableMethodArguments<>::apply<nsObserver...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/tuple:1077
1  xul.dll  std::apply(mozilla::detail::RunnableMethodArguments<>::apply<nsObserverServic...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/tuple:1088
1  xul.dll  mozilla::detail::RunnableMethodArguments<>::apply(nsObserverService*, void (n...  xpcom/threads/nsThreadUtils.h:1081

This crash signature aligns with the landing of bug 543435 so I suspect it might be a regression, Vincent can you have a look please?

Comment 1

[Vincent Hilla [:vhilla]]

My understanding is that we reach CheckURILoad in ReallyStartLoadingInternal with mTriggeringPrincipal being null and casting it to nsIPrincipal* crashes.

I don't see any obvious reason how bug 543435 could've affected that or why this could happen.

At the start of the method, code expects mTriggeringPrincipal can be null and there is a comment: https://searchfox.org/firefox-main/rev/d4456d810664a2a13a871a68e5323522a78c1131/dom/base/nsFrameLoader.cpp#657-660

The triggering principal could be null if the frame is loaded other than the src attribute, for example, the frame is sandboxed.

CheckURILoad also null checks aTriggeringPrincipal. Perhaps we can just pass null at the call site?

Comment 2

[Vincent Hilla [:vhilla]]

Oh but .get()ing mTriggeringPrincipal if it is null should be fine. Could the FrameLoader itself be null? It is strongly held in Document::mInitializableFrameLoaders.

I'll set S3 for now as it's low frequency.

Comment 3

[Vincent Hilla [:vhilla]]

Talking to :farre, we should tighten this debug assertion to a diagnostic assert. Then we know for sure that the issues is a nullptr in mInitializableFrameLoaders.

Comment 4

[Vincent Hilla [:vhilla]]

:smaug this is a wild guess, but do you remember something about this assert you added in 2008?

https://searchfox.org/firefox-main/rev/7496c8515212669451d7e775a00c2be07da38ca5/dom/base/Document.cpp#10058

while (mInitializableFrameLoaders.Length()) {
   RefPtr<nsFrameLoader> loader = mInitializableFrameLoaders[0];
   mInitializableFrameLoaders.RemoveElementAt(0);
   NS_ASSERTION(loader, "null frameloader in the array?");
   loader->ReallyStartLoading();
 }

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Well, we shouldn't ever have added null to the array, since that would be just weird.
And I still don't see how we could get a null pointer there, just looking at the two possible callers.

Comment 6

[BugBot [:suhaib / :marco]]

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

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #5)

And I still don't see how we could get a null pointer there, just looking at the two possible callers.

Filed bug 2007880 to get a bit of information related to this mystery.

Comment 8

[Henri Sivonen (:hsivonen)]

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #5)

And I still don't see how we could get a null pointer there, just looking at the two possible callers.

Filed bug 2007880 to get a bit of information related to this mystery.

...though it seems that the compiler is allowed to delete the assertion.

Comment 9

[Henri Sivonen (:hsivonen)]

It seems to me that nsFrameLoaderOwner::ChangeRemoteness is the only way to get a nullptr here.

On one hand, I fail to see what mechanism would ensure that mFrameLoader is non-null in nsFrameLoaderOwner::ChangeRemoteness. On the other hand, it seems really strange that if it is null, we'd get this far without an earlier null pointer crash.

The crash reports don't show anything in the "Crash inconsistencies" or "Possible bit flips max confidence" fields.

Comment 10

[Henri Sivonen (:hsivonen)]

Crash exists after bug 2007880 landed. Weird.

Comment 11

[Henri Sivonen (:hsivonen)]

TODO: Check if the microcode revisions shown in the crashes are non-latest.

Comment 12

[Henri Sivonen (:hsivonen)]

It looks like there are 5 distinct configs in the crash submissions. Very low crash rate. Unless I've missed something in code search, this should be an impossible null pointer from code inspection.

Comment 13

[Henri Sivonen (:hsivonen)]

It looks like bug 2007774 is the Linux counterpart of this bug with crash location attribution differing by platform.

Comment 14

[Henri Sivonen (:hsivonen)]

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

TODO: Check if the microcode revisions shown in the crashes are non-latest.

Crashes have both latest and non-latest, so probably that's not the explanation.

Comment 15

[Vincent Hilla [:vhilla]]

Attachment: Bug 2004647 - Assert mInitializableFrameLoaders doesn't contain null. r=hsivonen

Comment 16

[Phabricator Automation]

Comment on attachment 9537206 [details]
Bug 2004647 - Assert mInitializableFrameLoaders doesn't contain null. r=hsivonen

Revision D278805 was moved to bug 2010028. Setting attachment 9537206 [details] to obsolete.

Comment 17

[Henri Sivonen (:hsivonen)]

Expecting the crash location to change when bug 2010028 makes it into Nightly.

Comment 18

[Vincent Hilla [:vhilla]]

Bug 20010028 added MOZ_RELEASE_ASSERT(loader, "null frameloader in the array?"); and landed in 148.0b3 / 149.0a1 14.01.2026.

• There is no new crash with this signature: https://crash-stats.mozilla.org/search/?signature=~MaybeInitializeFinalizeFrameLoaders
• The current crash continues to be reported in those two versions. E.g. https://crash-stats.mozilla.org/report/index/8ec94046-ab33-4504-bf1a-d7a0a0260119

Comment 19

[BugBot [:suhaib / :marco]]

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

• Top 10 content process crashes on beta

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

For more information, please visit BugBot documentation.

Comment 20

[Vincent Hilla [:vhilla]]

Right now, it's 19th for 148b4 content crashes, 22nd for nightly. I think it's still low volume.

Comment 21

[Henri Sivonen (:hsivonen)]

Bug 2007774 landed, so we should expect to see the Windows crashes here to go away, too.

Comment 22

[Donal Meehan [:dmeehan]]

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

Bug 2007774 landed, so we should expect to see the Windows crashes here to go away, too.

Crash volume has gone as you expected. Can this bug be resolved?

Comment 23

[Henri Sivonen (:hsivonen)]

Yes. Resolving as duplicate of bug 2007774.