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

RESOLVED FIXED in Firefox 40

Status

()

--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bogus, 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

4 years ago
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

4 years ago
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

4 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

4 years ago
Blocks: 965413
(Assignee)

Updated

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

Comment 2

4 years ago
Posted patch crash fixSplinter Review
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)
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+
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

4 years ago
Posted patch for esr38Splinter Review
Flags: needinfo?(bugs)
Attachment #8630050 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

4 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 on attachment 8630050 [details] [diff] [review]
for esr38

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

Comment 11

4 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: 4 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

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

Comment 16

4 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+
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+
You need to log in before you can comment on or make changes to this bug.