1508044 - Assertion failure: !mIsThirdPartyTrackingResource, at netwerk/protocol/http/HttpBaseChannel.cpp:332

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

Attachment: assertion.log

https://www.youtube.com/?gl=FR&hl=fr

Firefox Nightly 65 / Linux. Not Beta 64.

Assertion failure: !mIsThirdPartyTrackingResource, at netwerk/protocol/http/HttpBaseChannel.cpp:332
#01: mozilla::net::HttpBackgroundChannelChild::RecvNotifyTrackingResource(bool const&) (netwerk/protocol/http/HttpBackgroundChannelChild.cpp:382)
#02: mozilla::net::PHttpBackgroundChannelChild::OnMessageReceived(IPC::Message const&) (firefox-debug/dist/include/mozilla/ipc/ProtocolUtils.h:397)
#03: mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) (firefox-debug/ipc/ipdl/PBackgroundChild.cpp:2280)
#04: mozilla::ipc::MessageChannel::AutoSetValue<int>::~AutoSetValue() (firefox-debug/dist/include/mozilla/ipc/MessageChannel.h:669)
#05: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (ipc/glue/MessageChannel.cpp:2171)
#06: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (ipc/glue/MessageChannel.cpp:2009)
#07: mozilla::ipc::MessageChannel::MessageTask::Run() (firefox-debug/dist/include/mozilla/Monitor.h:37)
#08: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:1231)
#09: NS_ProcessNextEvent(nsIThread*, bool) (xpcom/threads/nsThreadUtils.cpp:530)
[C

Comment 1

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7508d586f2c875555b6f79ad8d92057a53013229&tochange=3f5f46ca8ca0df1c34a9077fd4b479e291997a3f

https://hg.mozilla.org/integration/autoland/rev/3f5f46ca8ca0

+#ifdef NIGHTLY_BUILD
+// Use the strict list for the default cookie restrictions in Nightly
+pref("urlclassifier.trackingAnnotationTable", "test-track-simple,base-track-digest256,content-track-digest256");
+#else

Comment 2

[Johann Hofmann [:johannh]]

Ehsan, do you want to take a look at this crash?

Comment 3

[(no longer active)]

I couldn't reproduce this.  Bob, do you have any tips for how to reproduce?

Comment 4

[(no longer active)]

In the log, we have:

[Parent 14271, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001: file dom/base/ThirdPartyUtil.cpp, line 196
Assertion failure: !mIsThirdPartyTrackingResource, at netwerk/protocol/http/HttpBaseChannel.cpp:332

The failure is from <https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/netwerk/base/nsChannelClassifier.cpp#322>.  This tells me that this is related to bug 1456486.  Does that sound like a good assessment, Francois?

Comment 5

[François Marier [:francois]]

Based on the assertion: https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/netwerk/protocol/http/HttpBaseChannel.cpp#332

it looks like we call SetTracking twice for the same channel but with a different value for aIsThirdParty.

Another good reason to check the return values in the bug you linked to :)

Comment 6

[Bob Clary [:bc] (inactive)]

Try this at least on Linux.

mkdir /tmp/profile
firefox -profile /tmp/profile

should crash on load. Using an existing profile is much less reproducible.

Comment 7

[(no longer active)]

Aha, nice, that did the trick, thanks!

Comment 8

[(no longer active)]

Oh hmm I reproduced a different crash actually...

Comment 9

[(no longer active)]

May I ask which country you were running Firefox from?  It could be the interesting tracker is geo-specific...

Comment 10

[(no longer active)]

Or alternatively, can you please try out this patch to see if it fixes the crash?

Comment 11

[(no longer active)]

Attachment: Bug 1508044 - Check the return values of mozIThirdPartyUtil::IsThirdPartyFoo() in IsThirdParty() inside nsChannelClassifier.cpp

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: new assertion stack

US East Coast either from my home or the mdc2 data center.

Reproduced the original before your patch and got a different assertion with your patch:

Assertion failure: spec.LowerCaseEqualsLiteral("about:blank") || StringBeginsWith(spec, static_cast<const nsLiteralCString&>(nsLiteralCString("" "blob:"))) || nsContentUtils::StorageAllowedForWindow(aInnerWindow) == nsContentUtils::StorageAccess::eAllow>
#

Comment 13

[(no longer active)]

Great!  I think that means that my patch does indeed fix the bug you're seeing.  The second assertion is bug 1503787 which is unrelated (but important).  I'd appreciate if you could comment there so that Andrea could take a look.  (He has been trying to find a reproducible test case for that bug!)

Comment 14

[(no longer active)]

Hmm, this seems to be a lot more complex than I thought:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96102450def87f80812469fb632eb6cf49f5d2b2

On try we're actually hitting the assertion in comment 12.  Among other failures.

Perhaps this shows you can't just fix the problem for some of the call sites.  :-(

Comment 15

[(no longer active)]

This new version of the patch that I just uploaded should be a proper fix.  Can you please test this one Bob?

(Sorry to keep asking, I still have had no chance to reproduce the bug!  Thank you.)

Comment 16

[Bob Clary [:bc] (inactive)]

A quick rebuild after manipulating the patches via mq still reproduced the original assertion. I'm not sure if this is just a failure to rebuild properly. I'll clobber and rebuild. It takes me 45-60 minutes for a clobber build. I'll let you know when it is ready.

Comment 17

[(no longer active)]

Thank you for doing this, I highly suspect something could have gone wrong there...

Comment 18

[Bob Clary [:bc] (inactive)]

After a fresh pull and clobber...

Assertion failure: !mIsThirdPartyTrackingResource, at /home/bclary/mozilla/builds/nightly/mozilla/netwerk/protocol/http/HttpBaseChannel.cpp:332
#01: mozilla::net::HttpBackgroundChannelChild::RecvNotifyTrackingResource(bool const&) (/home/bclary/mozilla/builds/nightly/mozilla/netwerk/protocol/http/HttpBackgroundChannelChild.cpp:397)
#02: mozilla::net::PHttpBackgroundChannelChild::OnMessageReceived(IPC::Message const&) (/home/bclary/mozilla/builds/nightly/mozilla/firefox-debug/dist/include/mozilla/ipc/ProtocolUtils.h:397)
#03: mozilla::ipc::MessageChannel::AutoSetValue<int>::~AutoSetValue() (/home/bclary/mozilla/builds/nightly/mozilla/firefox-debug/dist/include/mozilla/ipc/MessageChannel.h:669)
#04: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (/home/bclary/mozilla/builds/nightly/mozilla/ipc/glue/MessageChannel.cpp:2173)
#05: mozilla::ipc::MessageChannel::MessageTask::Run() (/home/bclary/mozilla/builds/nightly/mozilla/firefox-debug/dist/include/mozilla/Monitor.h:37)
#06: nsThread::ProcessNextEvent(bool, bool*) (/home/bclary/mozilla/builds/nightly/mozilla/xpcom/threads/nsThread.cpp:1231)
#07: NS_ProcessNextEvent(nsIThread*, bool) (/home/bclary/mozilla/builds/nightly/mozilla/xpcom/threads/nsThreadUtils.cpp:530)
#08: mozilla::net::nsSocketTransportService::Run() (/home/bclary/mozilla/builds/nightly/mozilla/netwerk/base/nsSocketTransportService2.cpp:1083)
#09: non-virtual thunk to mozilla::net::nsSocketTransportService::Run() (:?)
#10: nsThread::ProcessNextEvent(bool, bool*) (/home/bclary/mozilla/builds/nightly/mozilla/xpcom/threads/nsThread.cpp:1231)
#11: NS_ProcessNextEvent(nsIThread*, bool) (/home/bclary/mozilla/builds/nightly/mozilla/xpcom/threads/nsThreadUtils.cpp:530)
#12: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/bclary/mozilla/builds/nightly/mozilla/ipc/glue/MessagePump.cpp:334)
#13: MessageLoop::AutoRunState::~AutoRunState() (/home/bclary/mozilla/builds/nightly/mozilla/ipc/chromium/src/base/message_loop.cc:598)
#14: nsThread::ThreadFunc(void*) (/home/bclary/mozilla/builds/nightly/mozilla/xpcom/threads/nsThread.cpp:505)

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: debug log

This might help with more context.

Comment 20

[(no longer active)]

Interesting.  Any chance you could run with this environment variable?

MOZ_LOG=nsChannelClassifier:5,thirdPartyUtil:5

My guess is that the top window URI is null here...

(I'd like to land this patch for now but I suspect a follow-up may be needed.)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4db66eb83f9
Check the return values of mozIThirdPartyUtil::IsThirdPartyFoo() in IsThirdParty() inside nsChannelClassifier.cpp r=francois

Comment 22

[Dorel Luca [:dluca]]

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

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: 1508044-mozlog.txt

from my build with your patch and a fresh empty profile again....
$ echo $MOZ_LOG
nsChannelClassifier:5,thirdPartyUtil:5

Comment 24

[(no longer active)]

Hmm, weird.  There is no log in the output at all.  Did you export MOZ_LOG?  Perhaps also try passing MOZ_LOG_FILES=foo.txt to see if you get log files generated and attach them here (both for the parent and child processes).

I looked at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging to see what could be wrong but didn't find anything helpful.

(BTW do you mind adding in nsHttp:5 to the MOZ_LOG variable too?  I realized that one will be useful to have as well.)

Last but not least I have been trying to reproduce this with a US-based VPN, still without luck.  :-(

Comment 25

[(no longer active)]

Wow, I can totally reproduce if I don't run under rr!

Comment 26

[(no longer active)]

Attachment: log.txt.bz2

Comment 27

[(no longer active)]

OK, the first call to SetIsTrackingResource() comes from here: <https://searchfox.org/mozilla-central/rev/6d748c47a82f59d75c22bf301177ad8fb10f786b/netwerk/protocol/http/TrackingDummyChannelChild.cpp#94>.  Here mIsThirdParty is true.  It comes from <https://searchfox.org/mozilla-central/rev/6d748c47a82f59d75c22bf301177ad8fb10f786b/dom/base/nsContentUtils.cpp#8984> where proper error checking isn't happening.

The second call is coming from <https://searchfox.org/mozilla-central/rev/6d748c47a82f59d75c22bf301177ad8fb10f786b/netwerk/protocol/http/HttpChannelChild.cpp#2105> with the correct value of aIsThirdParty but we assert because of that.

Comment 28

[(no longer active)]

(The reason this is triggered by the strict list is that the interesting channel is a youtube.com channel under a third-party which is itself under youtube.com, and youtube.com is on the strict list.

Comment 29

[(no longer active)]

But merely adding error checking doesn't fix the problem.  The real issue here is that the nature of the third-party check done on the channel here is different than what is done on the channel classifier call site.  Fixing that discrepancy fixes this assertion.

I have a patch series that unifies the two, pushed it to try to fix any possible test fixes and will then submit it for review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a2e659e560fd2577fce2376c8e6185a970e7b6

Thanks a lot Bob for your help in figuring this out!

Comment 30

[(no longer active)]

Attachment: Bug 1508044 - Part 2: Check the return values of mozIThirdPartyUtil::IsThirdPartyFoo() in nsContentUtils::IsThirdPartyWindowOrChannel()

Comment 31

[(no longer active)]

Attachment: Bug 1508044 - Part 3: Ensure that the third-party checks performed on channels in nsContentUtils::IsThirdPartyWindowOrChannel() follow the same logic as those performed in nsChannelClassifier

Depends on D12852

Comment 32

[(no longer active)]

Attachment: Bug 1508044 - Part 4: Make nsChannelClassifier use nsContentUtils::IsThirdPartyWindowOrChannel() in order to avoid having duplicated logic

Depends on D12853

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5452c9164d46
Part 2: Check the return values of mozIThirdPartyUtil::IsThirdPartyFoo() in nsContentUtils::IsThirdPartyWindowOrChannel() r=francois
https://hg.mozilla.org/integration/autoland/rev/691286d50c87
Part 3: Ensure that the third-party checks performed on channels in nsContentUtils::IsThirdPartyWindowOrChannel() follow the same logic as those performed in nsChannelClassifier r=francois
https://hg.mozilla.org/integration/autoland/rev/e3a074ae3f0c
Part 4: Make nsChannelClassifier use nsContentUtils::IsThirdPartyWindowOrChannel() in order to avoid having duplicated logic r=francois

Comment 34

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5452c9164d46
https://hg.mozilla.org/mozilla-central/rev/691286d50c87
https://hg.mozilla.org/mozilla-central/rev/e3a074ae3f0c

Comment 35

[Andrea Marchesini [:baku]]

> Reproduced the original before your patch and got a different assertion with
> your patch:

Are you still able to reproduce it consistently? if yes, do you have a STR starting from a clean profile?

Comment 36

[Bob Clary [:bc] (inactive)]

no longer reproduces for me locally and I haven't seen it in automation since the fix landed. Thanks!

Comment 37

[(no longer active)]

Thanks a lot for testing again!