Closed Bug 1483604 Opened Last year Closed Last year

Crash in static bool nsGlobalWindowOuter::SameLoadingURI

Categories

(Core :: DOM: Core & HTML, defect, critical)

Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: calixte, Assigned: ehsan)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

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
Flags: needinfo?(ehsan)
This isn't really a regression from bug 1482988, the code came from bug 1475697...

Taking.
Assignee: nobody → ehsan
Blocks: cookierestrictions, 1475697
No longer blocks: 1482988
Flags: needinfo?(ehsan)
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).
Attachment #9001409 - Flags: review?(bugs) → review+
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+
Attachment #9001410 - Flags: review?(bugs) → review+
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).
Attachment #9001411 - Flags: review?(bugs) → review+
(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
Crash Signature: [@ static bool nsGlobalWindowOuter::SameLoadingURI] → [@ static bool nsGlobalWindowOuter::SameLoadingURI] [@ nsGlobalWindowOuter::SameLoadingURI]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.