The URL classifier service tries to access the channel's top window URI in the parent process too soon

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Ehsan, Assigned: dimi)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

4 months ago

In the parent process, the channel's top window URI is unknown to us until we receive the asyncOpen() notification from the content process (https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/protocol/http/HttpChannelParent.cpp#491).

However the URL classifier service relies on this information sometimes before asyncOpen() has arrived, e.g. from BeginConnect().

Here is an example.

STR:

  1. ./mach mochitest toolkit/components/url-classifier/tests/browser/browser_flashblock_on_with_always_activate.js (in an optimized build this seems to reproduce better).
  2. We see the following in the console:

2:32.07 GECKO(21510) [Parent 21524, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/ehsan/moz/src/netwerk/url-classifier/UrlClassifierCommon.cpp, line 228
2:32.07 GECKO(21510) [Parent 21524, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp, line 686
2:32.07 GECKO(21510) [Parent 21524, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp, line 311
2:32.07 GECKO(21510) [Parent 21524, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp, line 548
2:32.07 GECKO(21510) [Parent 21524, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp, line 831

The failures come from this code: https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/netwerk/url-classifier/UrlClassifierCommon.cpp#293

Which gets called under this call stack:

#0  0x00007f17f5c33c37 in mozilla::net::HttpBaseChannel::GetTopWindowURI(nsIURI*, nsIURI**) (this=0x7f17c7bff000, aURIBeingLoaded=0x7f17c7b78200, aTopWindowURI=0x7ffc9c2334d8)
    at /home/ehsan/moz/src/netwerk/protocol/http/HttpBaseChannel.cpp:2355
#1  0x00007f17f5c33adf in mozilla::net::HttpBaseChannel::GetTopWindowURI(nsIURI**) (this=0x7f17c7bff000, aTopWindowURI=0x7ffc9c2334d8) at /home/ehsan/moz/src/netwerk/protocol/http/HttpBaseChannel.cpp:2338
#2  0x00007f17f5e2f416 in mozilla::net::UrlClassifierCommon::CreatePairwiseWhiteListURI(nsIChannel*, nsIURI**) (aChannel=0x7f17c7bff050, aURI=0x7ffc9c233838)
    at /home/ehsan/moz/src/netwerk/url-classifier/UrlClassifierCommon.cpp:227
#3  0x00007f17f5e36e6b in mozilla::net::UrlClassifierFeatureTrackingAnnotation::GetURIByListType(nsIChannel*, nsIUrlClassifierFeature::listType, nsIURI**)
    (this=0x7f17d0287bb0, aChannel=0x7f17c7bff050, aListType=nsIUrlClassifierFeature::listType::whitelist, aURI=0x7ffc9c233838)
    at /home/ehsan/moz/src/netwerk/url-classifier/UrlClassifierFeatureTrackingAnnotation.cpp:244
#4  0x00007f17f5e3b5cd in mozilla::net::(anonymous namespace)::FeatureData::InitializeList(mozilla::net::(anonymous namespace)::FeatureTask*, nsIChannel*, nsIUrlClassifierFeature::listType, nsTArray<RefPtr<mozilla::net::(anonymous namespace)::TableData> >&)
    (this=0x7f17c7b2ffb8, aTask=0x7f17c7b407c0, aChannel=0x7f17c7bff050, aListType=nsIUrlClassifierFeature::listType::whitelist, aList=nsTArray<RefPtr<mozilla::net::(anonymous namespace)::TableData> > &)
    at /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp:685
#5  0x00007f17f5e3aa04 in mozilla::net::(anonymous namespace)::FeatureData::Initialize(mozilla::net::(anonymous namespace)::FeatureTask*, nsIChannel*, nsIUrlClassifierFeature*)
    (this=0x7f17c7b2ffb8, aTask=0x7f17c7b407c0, aChannel=0x7f17c7bff050, aFeature=0x7f17d0287bb0) at /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp:309
#6  0x00007f17f5e2da36 in mozilla::net::(anonymous namespace)::FeatureTask::Create(nsIChannel*, std::function<void ()>&&, mozilla::net::(anonymous namespace)::FeatureTask**)
    (aChannel=0x7f17c7bff050, aCallback=..., aTask=0x7ffc9c233ba0) at /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp:547
#7  0x00007f17f5e2d4c7 in mozilla::net::AsyncUrlChannelClassifier::CheckChannel(nsIChannel*, std::function<void ()>&&) (aChannel=0x7f17c7bff050, aCallback=...)
    at /home/ehsan/moz/src/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp:830
#8  0x00007f17f5d2aba3 in mozilla::net::nsHttpChannel::BeginConnect() (this=0x7f17c7bff000) at /home/ehsan/moz/src/netwerk/protocol/http/nsHttpChannel.cpp:6563
#9  0x00007f17f5d2d035 in mozilla::net::nsHttpChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult)
    (this=0x7f17c7bff000, request=0x7f17d354fe60, channel=0x7f17c7bff050, pi=0x7f17c7b43120, status=nsresult::NS_OK) at /home/ehsan/moz/src/netwerk/protocol/http/nsHttpChannel.cpp:6873
#10 0x00007f17f57195fe in mozilla::net::nsAsyncResolveRequest::DoCallback()::{lambda(mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool)#1}::operator()(mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool) const (this=0x7f17c7b3f1b0, self=0x7f17d354fe50, pi=0x7f17c7b43120, async=false) at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:358
#11 0x00007f17f57197ea in std::_Function_handler<nsresult (mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool), mozilla::net::nsAsyncResolveRequest::DoCallback()::{lambda(mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool)#1}>::_M_invoke(std::_Any_data const&, mozilla::net::nsAsyncResolveRequest*&&, nsIProxyInfo*&&, bool&&)
    (__functor=..., __args=@0x7ffc9c234290: 0x7f17d354fe50, __args=@0x7ffc9c234288: 0x7f17c7b43120, __args=@0x7ffc9c234287: false)
    at /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/std_function.h:282
#12 0x00007f17f57074e1 in std::function<nsresult (mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool)>::operator()(mozilla::net::nsAsyncResolveRequest*, nsIProxyInfo*, bool) const
    (this=0x7f17c7b3f1b0, __args=0x7f17d354fe50, __args=0x7f17c7b43120, __args=false) at /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/std_function.h:687
#13 0x00007f17f56c9827 in mozilla::net::nsAsyncResolveRequest::AsyncApplyFilters::Finish() (this=0x7f17c7b3f120) at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:589
#14 0x00007f17f56c93e2 in mozilla::net::nsAsyncResolveRequest::AsyncApplyFilters::ProcessNextFilter() (this=0x7f17c7b3f120) at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:500
#15 0x00007f17f56c9115 in mozilla::net::nsAsyncResolveRequest::AsyncApplyFilters::AsyncProcess(mozilla::net::nsAsyncResolveRequest*) (this=0x7f17c7b3f120, aRequest=0x7f17d354fe50)
    at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:476
#16 0x00007f17f5718db7 in mozilla::net::nsAsyncResolveRequest::DoCallback() (this=0x7f17d354fe50) at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:367
#17 0x00007f17f5710ba8 in mozilla::net::nsAsyncResolveRequest::OnQueryComplete(nsresult, nsTSubstring<char> const&, nsTSubstring<char> const&)
    (this=0x7f17d354fe50, status=nsresult::NS_OK, pacString=..., newPACURL=...) at /home/ehsan/moz/src/netwerk/base/nsProtocolProxyService.cpp:314
#18 0x00007f17f56b48c3 in mozilla::net::ExecuteCallback::Run() (this=0x7f17d32662e0) at /home/ehsan/moz/src/netwerk/base/nsPACMan.cpp:119
#19 0x00007f17f54e286a in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f17ec36a4c0, aMayWait=false, aResult=0x7ffc9c234e67) at /home/ehsan/moz/src/xpcom/threads/nsThread.cpp:1162
#20 0x00007f17f54e5a53 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f17ec36a4c0, aMayWait=false) at /home/ehsan/moz/src/xpcom/threads/nsThreadUtils.cpp:474
#21 0x00007f17f6010c46 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f17ed31cd00, aDelegate=0x7f17ec32cc40) at /home/ehsan/moz/src/ipc/glue/MessagePump.cpp:88
#22 0x00007f17f5f3000f in MessageLoop::RunInternal() (this=0x7f17ec32cc40) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:315
#23 0x00007f17f5f2ff85 in MessageLoop::RunHandler() (this=0x7f17ec32cc40) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:308
#24 0x00007f17f5f2ff3a in MessageLoop::Run() (this=0x7f17ec32cc40) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:290
#25 0x00007f17fa8236d3 in nsBaseAppShell::Run() (this=0x7f17ec315120) at /home/ehsan/moz/src/widget/nsBaseAppShell.cpp:137
#26 0x00007f17fd5bbb82 in nsAppStartup::Run() (this=0x7f17ec35cb50) at /home/ehsan/moz/src/toolkit/components/startup/nsAppStartup.cpp:271
#27 0x00007f17fd7760c1 in XREMain::XRE_mainRun() (this=0x7ffc9c235920) at /home/ehsan/moz/src/toolkit/xre/nsAppRunner.cpp:4704
#28 0x00007f17fd776f34 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7ffc9c235920, argc=5, argv=0x7ffc9c236cd8, aConfig=...) at /home/ehsan/moz/src/toolkit/xre/nsAppRunner.cpp:4842
#29 0x00007f17fd777872 in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=5, argv=0x7ffc9c236cd8, aConfig=...) at /home/ehsan/moz/src/toolkit/xre/nsAppRunner.cpp:4926
#30 0x00007f17fd78ae47 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7f180684b6b0, argc=5, argv=0x7ffc9c236cd8, aConfig=...)
    at /home/ehsan/moz/src/toolkit/xre/Bootstrap.cpp:39
#31 0x000055bfe70187fc in do_main(int, char**, char**) (argc=5, argv=0x7ffc9c236cd8, envp=0x7ffc9c236d08) at /home/ehsan/moz/src/browser/app/nsBrowserApp.cpp:214

This all blows up because we don't have the top window URI yet, and we don't have a window in this process to get it directly from.

Reporter

Updated

4 months ago
Blocks: 1525458
Assignee

Updated

4 months ago
Priority: -- → P2
Reporter

Comment 1

4 months ago

It would be nice to see if this bug can occur in Firefox (outside of tests). If yes this is a super serious P1 bug, since it would mean the URL will be subject to no URL classification. If you pay attention to frame 6 in the call stack in comment 0, we're coming from FeatureTask::Create(), so bailing out early in this code can have disastrous consequences.

Assignee

Comment 2

4 months ago

Hi Ehsan, Thank you for the information, I'll check this.

Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee

Comment 3

4 months ago

Hi Ehasn,
I can't reproduce exactly the same scenario as yours but I did find something similar and I am working on it.

I wonder if you still have the log in the description and luckily with nsChannelClassifier log enabled?
I want to make sure I am checking the right stuff, thanks!

Assignee

Comment 4

4 months ago

(In reply to Dimi Lee [:dimi][:dlee] from comment #3)

I wonder if you still have the log in the description and luckily with nsChannelClassifier log enabled?
I want to make sure I am checking the right stuff, thanks!

I can reproduce it now, somehow this just doesn't show up when enabling log.

Assignee

Comment 5

4 months ago

(In reply to Dimi Lee [:dimi][:dlee] from comment #4)

I can reproduce it now, somehow this just doesn't show up when enabling log.

ah, I see...actually that is the root cause.

Assignee

Comment 6

4 months ago

The TopWindowURI is necessary for UrlClassifierCommon::ShouldEnableClassifier.
It shouldn't be in the UC_LOG_ENABLED block.

Assignee

Comment 7

4 months ago

Remove the workaround in Bug 1525458 because the issue is fixed.

Comment 8

4 months ago
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bdd564d2479
P1. Move the TopWindowURI check out of the UC_LOG_ENABLED check. r=baku
https://hg.mozilla.org/integration/autoland/rev/78f3aca43fde
P2. Remove previous testcase workaround. r=baku

Comment 9

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.