Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ] | Firefox 39.0 crashes immediately by javascript

RESOLVED FIXED in Firefox 40

Status

()

Core
Document Navigation
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Yuji Tounai, Assigned: smaug)

Tracking

({crash, csectype-nullptr, testcase})

39 Branch
mozilla42
All
Unspecified
crash, csectype-nullptr, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 wontfix, firefox40+ fixed, firefox41+ fixed, firefox42+ fixed, firefox-esr3840+ fixed)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8629580 [details]
when this file is opened by firefox,ff is crashed

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

I open the page like below,

----------
<!DOCTYPE html>
<html>
<body>
<script>
window.location.protocol = 'javascript';
</script>
</body>
</html>
----------
http://eastinside.org/ffcrash.html



Actual results:

Firefox crashes immediately and open the crash reporter.

I checked to crash on Firefox 39.0 and 38.0.5(Mac OS X/Windows 7 and
Firefox OS 2.2)

Comment 1

2 years ago
Created attachment 8629614 [details]
nightly debug crash report

bp-7121e4b6-a4de-460f-a7dc-1b0c72150705

debug assertion
Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsCOMPtr with operator->().), at ../dist/include/nsCOMPtr.h:706

aurora, beta, nightly - linux, osx, windows

Updated

2 years ago
Group: core-security
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ]
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
Component: Untriaged → Document Navigation
Ever confirmed: true
Product: Firefox → Core
Summary: Firefox 39.0 crashes immediately by javascript → Crash [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ] | Firefox 39.0 crashes immediately by javascript
Keywords: csectype-nullptr, testcase
Severity: normal → critical
Keywords: crash
Hardware: Unspecified → All
(Assignee)

Updated

2 years ago
Blocks: 965413
(Assignee)

Updated

2 years ago
Blocks: 1087442
No longer blocks: 965413
(Assignee)

Comment 2

2 years ago
Created attachment 8629660 [details] [diff] [review]
crash fix

Explicitly trying to not change the behavior from what we used to have, just
fix the crash.
Assignee: nobody → bugs
Attachment #8629660 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

2 years ago
It was http://hg.mozilla.org/mozilla-central/diff/c436f9b93cc9/dom/jsurl/nsJSProtocolHandler.cpp which skipped one NS_ENSURE_SUCCESS check.

Comment 4

2 years ago
Comment on attachment 8629660 [details] [diff] [review]
crash fix

>     if (NS_SUCCEEDED(rv)) {
>         channel.forget(result);

Pointless check here; should always be true, no?

r=me; we may want to backport this to 40 and 41...

I did check for when the potential leak bug 1087442 introduced here got fixed, and that was in bug 1153267 which is already in 40.  And in any case, the leak was only potential, since this SetLoadInfo call can never in fact fail in practice.
Attachment #8629660 - Flags: review?(bzbarsky) → review+

Comment 5

2 years ago
Oh, and we may want to backport to ESR38, but fix the maybe-leak while we're there....
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> It was
> http://hg.mozilla.org/mozilla-central/diff/c436f9b93cc9/dom/jsurl/
> nsJSProtocolHandler.cpp which skipped one NS_ENSURE_SUCCESS check.

I checked all the other ProtocolHandlers that perform init() before setLoadInfo() [1] to make sure we are not introducing the same error in other places, but that is not the case.

Thanks for fixing Olli!

[1] http://mxr.mozilla.org/mozilla-central/search?string=channel-%3ESetLoadInfo%28aLoadInfo%29%3B&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
(Assignee)

Comment 7

2 years ago
Created attachment 8630050 [details] [diff] [review]
for esr38
Flags: needinfo?(bugs)
Attachment #8630050 - Flags: review?(bzbarsky)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09937a4c395
(Assignee)

Comment 9

2 years ago
Comment on attachment 8629660 [details] [diff] [review]
crash fix

Approval Request Comment
[Feature/regressing bug #]: bug 1087442
[User impact if declined]: null pointer crash
[Describe test coverage new/current, TreeHerder]: crashtest
[Risks and why]: Should be safe. Crash becomes an exception
[String/UUID change made/needed]: NA
Attachment #8629660 - Flags: approval-mozilla-beta?
Attachment #8629660 - Flags: approval-mozilla-aurora?

Comment 10

2 years ago
Comment on attachment 8630050 [details] [diff] [review]
for esr38

r=me
Attachment #8630050 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

2 years ago
Comment on attachment 8630050 [details] [diff] [review]
for esr38

[Approval Request Comment]
See above for aurora/beta.
This fixes a null pointer crash.
Attachment #8630050 - Flags: approval-mozilla-esr38?
Setting nom for ESR38 tracking until the 40+ option is added to the list.
status-firefox39: affected → wontfix
status-firefox-esr38: --- → affected
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox-esr38: --- → ?
https://hg.mozilla.org/mozilla-central/rev/b09937a4c395
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
tracking-firefox-esr38: ? → 40+
Now that the fix is in a Nightly build, can you please try to reproduce again with the latest Nightly? If the fix is shown to be good, we'll get this into the Firefox 40 release.
Flags: needinfo?(bogus)

Comment 15

2 years ago
fyi, ran bughunter against the test case and only reproduced on beta/aurora.
(Reporter)

Comment 16

2 years ago
I'm not able to reproduce the crash with the latest Nightly.
It seems to be fixed.
Flags: needinfo?(bogus)
Comment on attachment 8629660 [details] [diff] [review]
crash fix

Good to see that we added a test case for it and that it was verified fixed on nightly. Seems safe to uplift to Aurora.
Attachment #8629660 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8629660 [details] [diff] [review]
crash fix

Discussed with Sylvestre, approving for uplift to Beta as well.
Attachment #8629660 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bea001f6780
status-firefox41: affected → fixed
Flags: in-testsuite+
https://hg.mozilla.org/releases/mozilla-beta/rev/447e511d00c7
status-firefox40: affected → fixed
Comment on attachment 8630050 [details] [diff] [review]
for esr38

Fix a crash, low risk, taking it.
Attachment #8630050 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
https://hg.mozilla.org/releases/mozilla-esr38/rev/b27fa5c1e8d9
status-firefox-esr38: affected → fixed
You need to log in before you can comment on or make changes to this bug.