Closed Bug 1180389 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Navigation, defect, critical)

39 Branch
All
Unspecified
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr38 40+ fixed

People

(Reporter: bogus, Assigned: smaug)

References

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(4 files)

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)
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
Group: core-security
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ]
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
Severity: normal → critical
Keywords: crash
Hardware: Unspecified → All
Blocks: 965413
Blocks: 1087442
No longer blocks: 965413
Attached 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
Attached patch for esr38Splinter Review
Flags: needinfo?(bugs)
Attachment #8630050 - Flags: review?(bzbarsky)
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+
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.
https://hg.mozilla.org/mozilla-central/rev/b09937a4c395
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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)
fyi, ran bughunter against the test case and only reproduced on beta/aurora.
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+
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.