1483765 - Assertion failure: !nsContentUtils::IsTrackingResourceWindow(aFirstPartyWindow)

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

STR:
1. Build a debug build on Windows (using clang)
2. Load https://d-toybox.com/studio/lib/input_event_viewer.html

Then, the tab is crashed due to hitting MOZ_ASSERT in AntiTrackingCommon::MaybeIsFirstPartyStorageAccessGrantedFor().

> Assertion failure: !nsContentUtils::IsTrackingResourceWindow(aFirstPartyWindow), at c:/mozilla/src/toolkit/components/antitracking/AntiTrackingCommon.cpp:468
> #01: mozilla::image::ImageCacheKey::ImageCacheKey (c:\mozilla\src\image\ImageCacheKey.cpp:51)
> #02: imgLoader::LoadImage (c:\mozilla\src\image\imgLoader.cpp:2350)
> #03: nsContentUtils::LoadImage (c:\mozilla\src\dom\base\nsContentUtils.cpp:3633)
> #04: mozilla::css::ImageLoader::LoadImage (c:\mozilla\src\layout\style\ImageLoader.cpp:386)
> #05: mozilla::css::ImageValue::Initialize (c:\mozilla\src\layout\style\nsCSSValue.cpp:1444)
> #06: nsStyleImageRequest::Resolve (c:\mozilla\src\layout\style\nsStyleStruct.cpp:2212)
> #07: nsStyleImage::ResolveImage (c:\mozilla\src\layout\style\nsStyleStruct.h:370)
> #08: nsStyleBackground::FinishStyle (c:\mozilla\src\layout\style\nsStyleStruct.cpp:3343)
> #09: mozilla::ComputedStyle::DoGetStyleBackground<1> (c:\mozilla\fx64-dbg\dist\include\nsStyleStructList.h:44)
> #10: nsCSSFrameConstructor::ConstructFramesFromItem (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:5974)
> #11: nsCSSFrameConstructor::ConstructFramesFromItemList (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:9991)
> #12: nsCSSFrameConstructor::ProcessChildren (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:10171)
> #13: nsCSSFrameConstructor::ConstructBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:11050)
> #14: nsCSSFrameConstructor::ConstructNonScrollableBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:4765)
> #15: nsCSSFrameConstructor::ConstructFrameFromItemInternal (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:3830)
> #16: nsCSSFrameConstructor::ConstructFramesFromItem (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:5984)
> #17: nsCSSFrameConstructor::ConstructFramesFromItemList (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:9991)
> #18: nsCSSFrameConstructor::ProcessChildren (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:10171)
> #19: nsCSSFrameConstructor::ConstructBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:11050)
> #20: nsCSSFrameConstructor::ConstructNonScrollableBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:4765)
> #21: nsCSSFrameConstructor::ConstructFrameFromItemInternal (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:3830)
> #22: nsCSSFrameConstructor::ConstructFramesFromItem (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:5984)
> #23: nsCSSFrameConstructor::ConstructFramesFromItemList (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:9991)
> #24: nsCSSFrameConstructor::ProcessChildren (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:10171)
> #25: nsCSSFrameConstructor::ConstructBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:11050)
> #26: nsCSSFrameConstructor::ConstructScrollableBlock (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:4713)
> #27: nsCSSFrameConstructor::ConstructFrameFromItemInternal (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:3830)
> #28: nsCSSFrameConstructor::ConstructFramesFromItem (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:5984)
> #29: nsCSSFrameConstructor::ConstructFramesFromItemList (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:9991)
> #30: nsCSSFrameConstructor::ContentAppended (c:\mozilla\src\layout\base\nsCSSFrameConstructor.cpp:7187)
> #31: mozilla::RestyleManager::ProcessRestyledFrames (c:\mozilla\src\layout\base\RestyleManager.cpp:1442)
> #32: mozilla::RestyleManager::DoProcessPendingRestyles (c:\mozilla\src\layout\base\RestyleManager.cpp:3058)
> #33: mozilla::PresShell::DoFlushPendingNotifications (c:\mozilla\src\layout\base\PresShell.cpp:4297)
> #34: nsIDocument::FlushPendingNotifications (c:\mozilla\src\dom\base\nsDocument.cpp:7445)
> #35: mozilla::dom::Element::GetPrimaryFrame (c:\mozilla\src\dom\base\Element.cpp:2356)
> #36: nsGenericHTMLElement::GetOffsetRect (c:\mozilla\src\dom\html\nsGenericHTMLElement.cpp:228)
> #37: mozilla::dom::HTMLElement_Binding::get_offsetWidth (c:\mozilla\fx64-dbg\dom\bindings\HTMLElementBinding.cpp:1157)
> #38: mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (c:\mozilla\src\d
> om\bindings\BindingUtils.cpp:3189)
> #39: CallJSNative (c:\mozilla\src\js\src\vm\Interpreter.cpp:445)
> #40: js::InternalCallOrConstruct (c:\mozilla\src\js\src\vm\Interpreter.cpp:533)
> #41: InternalCall (c:\mozilla\src\js\src\vm\Interpreter.cpp:584)
> #42: js::CallGetter (c:\mozilla\src\js\src\vm\Interpreter.cpp:723)
> #43: GetExistingProperty<js::CanGC> (c:\mozilla\src\js\src\vm\NativeObject.cpp:2174)
> #44: NativeGetPropertyInline<js::CanGC> (c:\mozilla\src\js\src\vm\NativeObject.cpp:2387)
> #45: js::NativeGetProperty (c:\mozilla\src\js\src\vm\NativeObject.cpp:2423)
> #46: js::GetProperty (c:\mozilla\src\js\src\vm\JSObject.h:788)
> #47: js::GetProperty (c:\mozilla\src\js\src\vm\Interpreter.cpp:4593)
> #48: Interpret (c:\mozilla\src\js\src\vm\Interpreter.cpp:2956)
> #49: js::RunScript (c:\mozilla\src\js\src\vm\Interpreter.cpp:425)
> #50: js::InternalCallOrConstruct (c:\mozilla\src\js\src\vm\Interpreter.cpp:557)
> #51: InternalCall (c:\mozilla\src\js\src\vm\Interpreter.cpp:584)
> #52: js::Call (c:\mozilla\src\js\src\vm\Interpreter.cpp:603)
> #53: js::fun_call (c:\mozilla\src\js\src\vm\JSFunction.cpp:1177)
> #54: CallJSNative (c:\mozilla\src\js\src\vm\Interpreter.cpp:445)
> #55: js::InternalCallOrConstruct (c:\mozilla\src\js\src\vm\Interpreter.cpp:533)
> #56: InternalCall (c:\mozilla\src\js\src\vm\Interpreter.cpp:584)
> #57: Interpret (c:\mozilla\src\js\src\vm\Interpreter.cpp:3239)
> #58: js::RunScript (c:\mozilla\src\js\src\vm\Interpreter.cpp:425)
> #59: js::InternalCallOrConstruct (c:\mozilla\src\js\src\vm\Interpreter.cpp:557)
> #60: InternalCall (c:\mozilla\src\js\src\vm\Interpreter.cpp:584)
> #61: js::Call (c:\mozilla\src\js\src\vm\Interpreter.cpp:603)
> #62: js::fun_call (c:\mozilla\src\js\src\vm\JSFunction.cpp:1177)
> #63: CallJSNative (c:\mozilla\src\js\src\vm\Interpreter.cpp:445)
> #64: js::InternalCallOrConstruct (c:\mozilla\src\js\src\vm\Interpreter.cpp:533)
> #65: InternalCall (c:\mozilla\src\js\src\vm\Interpreter.cpp:584)
> #66: Interpret (c:\mozilla\src\js\src\vm\Interpreter.cpp:3239)
> #67: js::RunScript (c:\mozilla\src\js\src\vm\Interpreter.cpp:425)
> #68: js::ExecuteKernel (c:\mozilla\src\js\src\vm\Interpreter.cpp:773)
> #69: js::Execute (c:\mozilla\src\js\src\vm\Interpreter.cpp:805)
> #70: ExecuteScript (c:\mozilla\src\js\src\jsapi.cpp:4657)
> #71: ExecuteScript (c:\mozilla\src\js\src\jsapi.cpp:4676)
> #72: nsJSUtils::ExecutionContext::CompileAndExec (c:\mozilla\src\dom\base\nsJSUtils.cpp:251)
> #73: mozilla::dom::ScriptLoader::EvaluateScript (c:\mozilla\src\dom\script\ScriptLoader.cpp:2389)
> #74: mozilla::dom::ScriptLoader::ProcessRequest (c:\mozilla\src\dom\script\ScriptLoader.cpp:2008)
> #75: mozilla::dom::ScriptLoader::ProcessInlineScript (c:\mozilla\src\dom\script\ScriptLoader.cpp:1608)
> #76: mozilla::dom::ScriptLoader::ProcessScriptElement (c:\mozilla\src\dom\script\ScriptLoader.cpp:1328)
> #77: mozilla::dom::ScriptElement::MaybeProcessScript (c:\mozilla\src\dom\script\ScriptElement.cpp:141)
> #78: nsHtml5TreeOpExecutor::RunScript (c:\mozilla\src\parser\html\nsHtml5TreeOpExecutor.cpp:738)
> #79: nsHtml5TreeOpExecutor::RunFlushLoop (c:\mozilla\src\parser\html\nsHtml5TreeOpExecutor.cpp:540)
> #80: nsHtml5ExecutorFlusher::Run (c:\mozilla\src\parser\html\nsHtml5StreamParser.cpp:123)
> #81: nsThread::ProcessNextEvent (c:\mozilla\src\xpcom\threads\nsThread.cpp:1182)
> #82: NS_ProcessNextEvent (c:\mozilla\src\xpcom\threads\nsThreadUtils.cpp:519)
> #83: mozilla::ipc::MessagePump::Run (c:\mozilla\src\ipc\glue\MessagePump.cpp:97)
> #84: MessageLoop::RunHandler (c:\mozilla\src\ipc\chromium\src\base\message_loop.cc:319)
> #85: MessageLoop::Run (c:\mozilla\src\ipc\chromium\src\base\message_loop.cc:299)
> #86: nsBaseAppShell::Run (c:\mozilla\src\widget\nsBaseAppShell.cpp:160)
> #87: nsAppShell::Run (c:\mozilla\src\widget\windows\nsAppShell.cpp:415)
> #88: nsAppStartup::Run (c:\mozilla\src\toolkit\components\startup\nsAppStartup.cpp:291)
> #89: XREMain::XRE_mainRun (c:\mozilla\src\toolkit\xre\nsAppRunner.cpp:4799)
> #90: XREMain::XRE_main (c:\mozilla\src\toolkit\xre\nsAppRunner.cpp:4944)
> #91: XRE_main (c:\mozilla\src\toolkit\xre\nsAppRunner.cpp:5036)
> #92: NS_internal_main (c:\mozilla\src\browser\app\nsBrowserApp.cpp:311)
> #93: wmain (c:\mozilla\src\toolkit\xre\nsWindowsWMain.cpp:143)
> #94: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
> #95: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x13034]
> #96: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x71431]

Comment 1

[(no longer active)]

The assertion comes from this call site: <https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/image/ImageCacheKey.cpp#160>

The basic failure mode here is that we call nsContentUtils::StorageDisabledByAntiTracking() first which calls AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor() internally, and it bails out early here <https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/toolkit/components/antitracking/AntiTrackingCommon.cpp#253> but then we go ahead to call AntiTrackingCommon::MaybeIsFirstPartyStorageAccessGrantedFor() which makes the wrong assumption: <https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/toolkit/components/antitracking/AntiTrackingCommon.cpp#468>.  And indeed, IsFirstPartyStorageAccessGrantedFor() can return early for many reasons.

The real thing that caused this regression was bug 1480780 though, which removed a check for IsTrackingResourceWindow() from the image cache call site <https://searchfox.org/mozilla-central/diff/d1e5833a37e440e75eca67cbb72748a36fb16c9b/image/ImageCacheKey.cpp#147>.  But it also simultaneously moved down the check for IsTrackingResourceWindow() further down IsFirstPartyStorageAccessGrantedFor(), and as a result, there is no reason why this assertion would hold any longer, and indeed it does not.

Comment 2

[(no longer active)]

Attachment: Fix the image cache key computation logic to only consider first-party storage access for third-party windows

The test simulates the scenario that the page in comment 0 triggered: A tracking iframe inside a normal top-level page having an SVG image used as a background image.  The test merely ensures that we don't crash while loading this page with the right prefs in debug mode.

The fix to the bug is simple, it fixes the condition in the image cache key computation that bug 1480780 accidentally broke.

Comment 3

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

Comment on attachment 9001760 [details] [diff] [review]
Fix the image cache key computation logic to only consider first-party storage access for third-party windows

It is unclear to me why nsContentUtils::StorageDisabledByAntiTracking doesn't internally check nsContentUtils::IsTrackingResourceWindow. 
That is how I would expect method named as StorageDisabledByAntiTracking to work.
But this should be ok too, for now.

I sure hope storage isn't ever disabled on non-tracking-window.

But in the test, I don't understand what obj.svg refers to.

Comment 4

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 9001760 [details] [diff] [review]
> Fix the image cache key computation logic to only consider first-party
> storage access for third-party windows
> 
> It is unclear to me why nsContentUtils::StorageDisabledByAntiTracking
> doesn't internally check nsContentUtils::IsTrackingResourceWindow. 
> That is how I would expect method named as StorageDisabledByAntiTracking to
> work.

It does, already.  This function calls AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor() <https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/dom/base/nsContentUtils.cpp#8877> which in turn performs the check you're referring to: <https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/toolkit/components/antitracking/AntiTrackingCommon.cpp#252>.

> But this should be ok too, for now.
> 
> I sure hope storage isn't ever disabled on non-tracking-window.

The issue here is that the image cache caller actually cares about three cases and not just two:

  a) Tracking third-party windows
  b) Tracking subresources of a non-tracking first-party window
  c) Non-tracking windows (first-party or third-party)

AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor() however returns only a boolean, so it can inform its caller of at most two possibilities (whether the window passed to it had storage access for the purpose of loading the URI).  The problem is that it could return false either due to cases (b) or (c) above, and the consumer has no way of knowing which case happened.

The patch here basically makes the first if branch check whether (a) has happened, and the second one check whether (b) has happened, and if neither happen we know that (c) has happened.

Note that we don't have access to the channel here, otherwise we would be able to recognize a third-party channel, and effectively recognize (b) here.  The comments here explain why the image cache caller is special and why this isn't needed anywhere else <https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/image/ImageCacheKey.cpp#150>.  Other consumers that want to distinguish between (b) and (c) do so by passing the channel object for the load, which allows IsFirstPartyStorageAccessGrantedFor() to recognize a third-party tracker subresource load.

> But in the test, I don't understand what obj.svg refers to.

Oh, sorry, that's left over from an earlier attempt at making the test work, I'll rip it out.

Comment 5

[(no longer active)]

Attachment: Fix the image cache key computation logic to only consider first-party storage access for third-party windows

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7d388314b8
Fix the image cache key computation logic to only consider first-party storage access for third-party windows; r=smaug

Comment 7

[(no longer active)]

Backed out because of a crash with SVG image documents...  I think I need to move the cookie aversion check to earlier in the function.

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc839f3cdd7
Backout changeset 2c7d388314b8 for crashes on a CLOSED TREE

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22cb98ed738b
Fix the image cache key computation logic to only consider first-party storage access for third-party windows; r=smaug

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/22cb98ed738b