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

Thanks, I am looking into it.

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.

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?

I believe so, I will check everywhere and submit another patch for it.

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?

just happened to me in Thunderbird bp-e0087bd4-3302-4d88-80c4-e545c2170207. 20170203 nightly build does not crash.

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.

Try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8f20ac338e&selectedJob=75166794

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

https://hg.mozilla.org/mozilla-central/rev/9601b5036458 https://hg.mozilla.org/mozilla-central/rev/70debab47688

Per IRC discussion with Tim, this doesn't need to be considered for backport.