This bug was filed from the Socorro interface and is report bp-d9361a7d-a69d-4e20-a3e5-824540180815. ============================================================= Top 10 frames of crashing thread: 0 xul.dll static bool nsGlobalWindowOuter::SameLoadingURI dom/base/nsGlobalWindowOuter.cpp:5309 1 xul.dll nsGlobalWindowOuter::NotifyContentBlockingState dom/base/nsGlobalWindowOuter.cpp:5276 2 xul.dll nsContentUtils::StorageDisabledByAntiTracking dom/base/nsContentUtils.cpp:8920 3 xul.dll mozilla::image::ImageCacheKey::ImageCacheKey image/ImageCacheKey.cpp:51 4 xul.dll imgLoader::LoadImage image/imgLoader.cpp:2349 5 xul.dll mozilla::css::ImageLoader::LoadImage layout/style/ImageLoader.cpp:379 6 xul.dll nsStyleImageRequest::Resolve layout/style/nsStyleStruct.cpp:2211 7 xul.dll nsStyleImageLayers::ResolveImages layout/style/nsStyleStruct.h:735 8 xul.dll void nsCSSFrameConstructor::ConstructFramesFromItem layout/base/nsCSSFrameConstructor.cpp:5972 9 xul.dll struct already_AddRefed<mozilla::ComputedStyle> nsCSSFrameConstructor::BeginBuildingScrollFrame layout/base/nsCSSFrameConstructor.cpp:4397 ============================================================= There are 3 crashes (from 2 installations) in nightly 63 starting with buildid 20180814220344. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1482988. [1] https://hg.mozilla.org/mozilla-central/rev?node=f93a51060a07

This isn't really a regression from bug 1482988, the code came from bug 1475697... Taking.

The simple fix here, of course, is to null check the channel argument. But that means that everywhere that aChannel is null, we won't be notifying the UI, which is not all that great. With a little bit of more work, we could use the document's channel in many existing call sites to pass down a channel object that this code could use when notifying the UI, therefore making it mostly work. Parts 2 and 3 do that. With those fixed, there will be only a few call sites that pass in a null channel object knowingly: callers of nsContentUtils::StorageAllowedForNewWindow() (1 caller) and callers of nsContentUtils::StorageAllowedForPrincipal() (2 callers).

Comment on attachment 9001410 [details] [diff] [review] Part 2: Make all of the external consumers of nsContentUtils::StorageDisabledByAntiTracking() pass a channel if available >+ /* >+ * Returns true if this document should disable storages because of the anti-tracking feature. >+ */ >+ static bool StorageDisabledByAntiTracking(nsIDocument* aDocument, >+ nsIURI* aURI) >+ { >+ return StorageDisabledByAntiTracking(aDocument->GetInnerWindow(), >+ aDocument->GetChannel(), >+ aDocument->NodePrincipal(), >+ aURI); >+ } I'm trying to think what does it mean when aDocument->GetChannel() returns null. (like with initial about:blank, IIRC) Could you explain why this is ok with initial about:blank? with that, r+

Comment on attachment 9001411 [details] [diff] [review] Part 3: Pass in a channel object when calling InternalStorageAllowedForPrincipal() from StorageAllowedForWindow() and StorageAllowedForDocument() Also here, what if document doesn't have channel? Still looks reasonable, but the edge case might or might not be ok. Please explain (or correct me that initial about:blank gets channel after all).

(In reply to Olli Pettay [:smaug] from comment #6) > Comment on attachment 9001410 [details] [diff] [review] > Part 2: Make all of the external consumers of > nsContentUtils::StorageDisabledByAntiTracking() pass a channel if available > > > >+ /* > >+ * Returns true if this document should disable storages because of the anti-tracking feature. > >+ */ > >+ static bool StorageDisabledByAntiTracking(nsIDocument* aDocument, > >+ nsIURI* aURI) > >+ { > >+ return StorageDisabledByAntiTracking(aDocument->GetInnerWindow(), > >+ aDocument->GetChannel(), > >+ aDocument->NodePrincipal(), > >+ aURI); > >+ } > I'm trying to think what does it mean when aDocument->GetChannel() returns > null. (like with initial about:blank, IIRC) > > Could you explain why this is ok with initial about:blank? StorageDisabledByAntiTracking() first tries to use the innerwindow argument, and if that's null it falls back to the channel object to do its main work. Besides that, the channel argument being passed in (when innerwindow is non-null) will only be used for reporting the security UI notifications, so when the channel argument is null, it is only the reporting part that won't get done. In other words, the reporting is a best-effort attempt. This change only makes the channel object available for reporting in the cases where it's non-null (e.g. in the case of the initial about:blank, the reporting won't happen.) I'll clarify this in a comment.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/05a3c486ad1e Part 1: Only call NotifyContentBlockingState() when the channel argument is non-null; r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/be2d05373754 Part 2: Make all of the external consumers of nsContentUtils::StorageDisabledByAntiTracking() pass a channel if available; r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9aaa303e9a Part 3: Pass in a channel object when calling InternalStorageAllowedForPrincipal() from StorageAllowedForWindow() and StorageAllowedForDocument(); r=smaug

https://hg.mozilla.org/mozilla-central/rev/05a3c486ad1e https://hg.mozilla.org/mozilla-central/rev/be2d05373754 https://hg.mozilla.org/mozilla-central/rev/6b9aaa303e9a