1611907 - Crash in [@ mozilla::net::nsHttpChannel::AssertNotDocumentChannel]

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Cristian Fogel [:cfogel]]

This bug is for crash report bp-199adb68-6821-4016-a106-3f24b0200128.

Top 10 frames of crashing thread:

0 XUL mozilla::net::nsHttpChannel::AssertNotDocumentChannel netwerk/protocol/http/nsHttpChannel.cpp:2638
1 XUL mozilla::net::nsHttpChannel::ContinueProcessResponse1 netwerk/protocol/http/nsHttpChannel.cpp:2521
2 XUL mozilla::net::nsHttpChannel::ProcessResponse netwerk/protocol/http/nsHttpChannel.cpp:2477
3 XUL mozilla::net::nsHttpChannel::OnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:7794
4 XUL nsInputStreamPump::OnStateStart netwerk/base/nsInputStreamPump.cpp:504
5 XUL nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:413
6 XUL nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:89
7 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1220
8 XUL NS_ProcessPendingEvents xpcom/threads/nsThreadUtils.cpp:434
9 XUL nsBaseAppShell::NativeEventCallback widget/nsBaseAppShell.cpp:87

Affected versions

• 74.0a1 (2020-01-27);

Affected platforms

• Windows 10, Ubuntu 18.0, macOS 10.15;

Preconditions - prefs are set to true:

• browser.tabs.documentchannel
• dom.serviceWorkers.parent_intercept
• fission.rebuild_frameloaders_on_remoteness_change

Steps to reproduce

1. Launch Firefox;
2. Access about:config and set fission.autostart to true;
3. Restart the browser;
4. Access https://edition.cnn.com/
5. Click [Accept] on the cookies prompt from the bottom of the screen;
6. Scroll down, click on any article (for ex, one from Special Report)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Given that dom.serviceWorkers.parent_intercept is needed, it's probably a SW issue.
Andrew, can you take a look?

Comment 2

[Cristian Fogel [:cfogel]]

Managed to reproduce the issue with the setup bellow as well, if it's of any help.

• fission.autostart - true
• browser.tabs.documentchannel - true
• dom.serviceWorkers.parent_intercept - false
• fission.rebuild_frameloaders_on_remoteness_change - false

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(In reply to Cristian Fogel, QA [:cfogel] from comment #2)

Managed to reproduce the issue with the setup bellow as well, if it's of any help.

Thanks for the info. That means it's probably not related to service-workers.

Comment 4

[Chris Peterson [:cpeterson]]

Matt, does this AssertNotDocumentChannel assertion failure indicate a bug in DocumentChannel itself or in the caller?

Comment 5

[Cristian Fogel [:cfogel]]

Regression range:

• Pushlog URL
• Last good build_date: 2019-11-13
• First bad build_date: 2019-11-14

Comment 6

[Cristian Fogel [:cfogel]]

Since 1593545 is in the pushlog as well for this particular issue, any chance it might the regressor?

Comment 7

[Paul Bone [:pbone]]

That change only disabled that assertion for cases when it wasn't important (when document channel is disabled). It shouldn't have affected the case when it's enabled. But thanks for the NI. I guess Matt or I will investigate more.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Thanks Paul!

Comment 9

[Chris Peterson [:cpeterson]]

74=disabled because Matt says this assert is conditional on Fission being enabled, which is only allowed in Nightly.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

:cfogel, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 11

[Cristian Fogel [:cfogel]]

Waiting for the input from Matt.

Comment 12

[Intermittent Failures Robot]

1 failures in 5233 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• android-em-7-0-x86_64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1611907&startday=2020-02-24&endday=2020-03-01&tree=all

Comment 13

[Jens Stutte [:jstutte]]

Hi Nhi, can we find an owner/assignee for this bug such that it can make it into 75? Thank you!

Comment 14

[Nhi Nguyen (:nhi)]

Paul, could you take this bug, seeing that it's a regression from bug 1588412?

Comment 15

[Paul Bone [:pbone]]

mattwoodrow, I have some thoughts, and questions at the bottom.

The load that hits the assertion looks like it's for a tracking pixel, (my guess based on the URL):

"https://widgets.outbrain.com/nanoWidget/externals/
obPixelFrame/obPixelFrame.htm#p=https%3A%2F%2Famplify-imp.outbrain.com%2Fpixel%3
Fp%3DnlV1YHXXXKgnJTkmjxGkpGf4XrSS3FF4rFrjT6iBnylEGk5z_ll-xTJhYcCB6tivs"

Loaded as part of the document, I don't know how to find out in gdb if it's an iframe or not. I'd normally use devtools but the tab is crashed ;-).

The nsHttpChannel::mListener property points to a ParentProcessDocumentOpenInfo so I think that's being created here:

https://searchfox.org/mozilla-central/source/netwerk/ipc/DocumentLoadListener.cpp#462-464

We're hitting the assertion because neither of these conditions are true:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2674-2688

So I guess what's happening is that the parent (mListener, I think they're the same) is ParentProcessDocumentOpenInfo and neither of these conditions succeeds. That makes it look like a parent load. Should that URL (probably a tracking pixel although that's still a guess) be a parent load?

Should I fix this by checking if the parent is a ParentProcessDocumentOpenInfo in AssertNotDocumentChannel?

Thanks.

Comment 16

[Matt Woodrow (:mattwoodrow)]

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

Hi Nhi, can we find an owner/assignee for this bug such that it can make it into 75? Thank you!

As mentioned earlier, this is Nightly+fission only, so won't be riding the trains, and we don't need to track it.

(In reply to Paul Bone [:pbone] from comment #15)

So I guess what's happening is that the parent (mListener, I think they're the same) is ParentProcessDocumentOpenInfo and neither of these conditions succeeds. That makes it look like a parent load. Should that URL (probably a tracking pixel although that's still a guess) be a parent load?

NS_QueryNotificationCallbacks queries mCallbacks, not mListener, which should be the DocumentLoadListener. If that didn't work, then this would be crashing for every http load with fission enabled.

Comment 17

[Matt Woodrow (:mattwoodrow)]

This crash is happening roughly once a week. I'd rather just remove the assertion than invest more time into it, since I don't think the result matters too much.

Comment 18

[Paul Bone [:pbone]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

This crash is happening roughly once a week. I'd rather just remove the assertion than invest more time into it, since I don't think the result matters too much.

it's fine to remove assertions if maintaining them is not worth the effort. But I'd like to understand what's happening first. If this is a hint that we've made an error elsewhere we should follow up on it.

The mCallbacks field points to a ClientChannelHelper, which is related to document channel, I guess that means that this load is going through document channel.

Comment 19

[Paul Bone [:pbone]]

Attachment: Bug 1611907 - Remove AssertNotDocumentChannel r=mattwoodrow

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc2b36de8f33
Remove AssertNotDocumentChannel r=mattwoodrow

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:pbone, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

[Paul Bone [:pbone]]

No it's not.

The assertion can only happen if FIssion is enabled. Fission should not be enabled in Beta (there's other warnings and stuff to persuade users not to do this). Fission can only be enabled from about:config anyway.

Comment 24

[Cristian Fogel [:cfogel]]

Verified with 76.0a1 (2020-03-30) on Windows 10, macOS 10.15, Ubuntu 18.04.
Did some additional checks on Windows 10/macOS 10.15 as in navigating around pages to ensure no crashes occur; all good.