Crash in nsILoadInfo::GetOriginAttributes

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Security
P1
critical
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: marcia, Assigned: timhuang)

Tracking

({crash, regression})

Trunk
mozilla54
x86
Windows 8
crash, regression
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

(Whiteboard: [domsecurity-active][tbird topcrash], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

This bug was filed from the Socorro interface and is 
report bp-0479cd2c-a0a6-4132-a393-ce2c22170205.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170204030205 build: http://bit.ly/2k9ZKNF

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2aede0a97bc685e163196cc451b947a04ae6a598&tochange=7aa5e444af0ff686714d6165bd0e7e6d1abd0970

Bug 1312954 is the range. ni on timhuang
Flags: needinfo?(tihuang)
(Assignee)

Comment 1

9 months ago
Thanks, I am looking into it.
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Flags: needinfo?(tihuang)
(Assignee)

Comment 2

9 months ago
It looks like sometimes the channel here doesn't have a loadInfo, maybe this channel came from an addon. For this case, we should check the loadInfo is null or not.
Set as P1 since it's both a crash and a regression.
Keywords: regression
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8833897 [details]
Bug 1336802 - Part 1: Fixing the crash of nsILoadInfo::GetOriginAttributes.

https://reviewboard.mozilla.org/r/110022/#review111018

Do we need similar null checks also elsewhere?
Attachment #8833897 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

9 months ago
I believe so, I will check everywhere and submit another patch for it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

9 months ago
mozreview-review
Comment on attachment 8834256 [details]
Bug 1336802 - Part 2: Updating the whole code base to make sure nsILoadInfo get null check.

https://reviewboard.mozilla.org/r/110260/#review111470

::: dom/base/nsObjectLoadingContent.cpp:2598
(Diff revision 1)
>                       nsIChannel::LOAD_CLASSIFY_URI |
>                       nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (inherit) {
>      nsCOMPtr<nsILoadInfo> loadinfo = chan->GetLoadInfo();
> +    if (loadinfo) {

I think if we don't have loadinfo here, we should fail.
So, perhaps just add
NS_ENSURE_STATE(loadinfo);
after 
nsCOMPtr<nsILoadInfo> loadinfo = chan->GetLoadInfo();

::: dom/html/nsHTMLDocument.cpp:2321
(Diff revision 1)
>                       NodePrincipal(),
>                       nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL,
>                       nsIContentPolicy::TYPE_OTHER);
>    NS_ENSURE_SUCCESS(rv, rv);
>    nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> +  if (loadInfo) {

Also here, 
NS_ENSURE_STATE(loadInfo);

::: dom/security/nsMixedContentBlocker.cpp:387
(Diff revision 1)
> -    if (NS_FAILED(rv)) {
> +      if (NS_FAILED(rv)) {
> -      decision = REJECT_REQUEST;
> +        decision = REJECT_REQUEST;
> -      newLoadInfo->ClearHSTSPriming();
> +        newLoadInfo->ClearHSTSPriming();
> -    }
> +      }
> -  }
> +    }
> +  }

I think we should REJECT here if there is no loadinfo

::: dom/workers/ServiceWorkerEvents.cpp:193
(Diff revision 1)
>      nsresult rv = mChannel->GetChannel(getter_AddRefs(underlyingChannel));
>      NS_ENSURE_SUCCESS(rv, rv);
>      NS_ENSURE_TRUE(underlyingChannel, NS_ERROR_UNEXPECTED);
>      nsCOMPtr<nsILoadInfo> loadInfo = underlyingChannel->GetLoadInfo();
>  
> -    if (!CSPPermitsResponse(loadInfo)) {
> +    if (loadInfo && !CSPPermitsResponse(loadInfo)) {

this should probably be
if (!loadInfo || !CSPPermitsResponse(loadInfo))

::: dom/workers/ServiceWorkerPrivate.cpp:1377
(Diff revision 1)
>      uint32_t loadFlags;
>      rv = channel->GetLoadFlags(&loadFlags);
>      NS_ENSURE_SUCCESS(rv, rv);
>      nsCOMPtr<nsILoadInfo> loadInfo;
>      rv = channel->GetLoadInfo(getter_AddRefs(loadInfo));
>      NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_STATE?
Attachment #8834256 - Flags: review?(bugs) → review-
Whiteboard: [domsecurity-active]
Comment hidden (mozreview-request)

Comment 11

9 months ago
just happened to me in Thunderbird bp-e0087bd4-3302-4d88-80c4-e545c2170207. 20170203 nightly build does not crash.
Blocks: 1337108
Whiteboard: [domsecurity-active] → [domsecurity-active][tbird topcrash]

Comment 12

9 months ago
mozreview-review
Comment on attachment 8834256 [details]
Bug 1336802 - Part 2: Updating the whole code base to make sure nsILoadInfo get null check.

https://reviewboard.mozilla.org/r/110260/#review112150

r+, but this is worrisome. We really should get into state where the null checks aren't needed.
Attachment #8834256 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

9 months ago
Try looks good. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8f20ac338e&selectedJob=75166794
Keywords: checkin-needed

Comment 14

9 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9601b5036458
Part 1: Fixing the crash of nsILoadInfo::GetOriginAttributes. r=smaug
https://hg.mozilla.org/integration/autoland/rev/70debab47688
Part 2: Updating the whole code base to make sure nsILoadInfo get null check. r=smaug
Keywords: checkin-needed

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9601b5036458
https://hg.mozilla.org/mozilla-central/rev/70debab47688
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Per IRC discussion with Tim, this doesn't need to be considered for backport.
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected

Updated

7 months ago
Duplicate of this bug: 1337108
You need to log in before you can comment on or make changes to this bug.