Last Comment Bug 1180389 - Crash [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ] | Firefox 39.0 crashes immediately by javascript
: Crash [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ] | Firefox 39.0 crashes imme...
Status: RESOLVED FIXED
: crash, csectype-nullptr, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: 39 Branch
: All Unspecified
: -- critical (vote)
: mozilla42
Assigned To: Olli Pettay [:smaug] (review request backlog because of a work week)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 1087442
  Show dependency treegraph
 
Reported: 2015-07-04 06:09 PDT by yujitounai
Modified: 2015-07-16 11:25 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
40+
fixed


Attachments
when this file is opened by firefox,ff is crashed (108 bytes, text/html)
2015-07-04 06:09 PDT, yujitounai
no flags Details
nightly debug crash report (131.29 KB, text/plain)
2015-07-04 17:14 PDT, Bob Clary [:bc:]
no flags Details
crash fix (1.18 KB, patch)
2015-07-05 05:41 PDT, Olli Pettay [:smaug] (review request backlog because of a work week)
bzbarsky: review+
rkothari: approval‑mozilla‑aurora+
rkothari: approval‑mozilla‑beta+
Details | Diff | Splinter Review
for esr38 (1.70 KB, patch)
2015-07-06 10:28 PDT, Olli Pettay [:smaug] (review request backlog because of a work week)
bzbarsky: review+
sledru: approval‑mozilla‑esr38+
Details | Diff | Splinter Review

Description User image yujitounai 2015-07-04 06:09:41 PDT
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 User image Bob Clary [:bc:] 2015-07-04 17:14:44 PDT
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
Comment 2 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2015-07-05 05:41:24 PDT
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.
Comment 3 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2015-07-05 05:43:26 PDT
It was http://hg.mozilla.org/mozilla-central/diff/c436f9b93cc9/dom/jsurl/nsJSProtocolHandler.cpp which skipped one NS_ENSURE_SUCCESS check.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-05 22:29:30 PDT
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.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-05 22:30:13 PDT
Oh, and we may want to backport to ESR38, but fix the maybe-leak while we're there....
Comment 6 User image Christoph Kerschbaumer [:ckerschb] 2015-07-06 08:54:16 PDT
(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
Comment 7 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2015-07-06 10:28:39 PDT
Created attachment 8630050 [details] [diff] [review]
for esr38
Comment 9 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2015-07-06 10:33:10 PDT
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
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-06 10:51:31 PDT
Comment on attachment 8630050 [details] [diff] [review]
for esr38

r=me
Comment 11 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2015-07-06 11:21:50 PDT
Comment on attachment 8630050 [details] [diff] [review]
for esr38

[Approval Request Comment]
See above for aurora/beta.
This fixes a null pointer crash.
Comment 12 User image Lawrence Mandel [:lmandel] (use needinfo) 2015-07-06 11:52:45 PDT
Setting nom for ESR38 tracking until the 40+ option is added to the list.
Comment 13 User image Wes Kocher (:KWierso) 2015-07-07 13:01:14 PDT
https://hg.mozilla.org/mozilla-central/rev/b09937a4c395
Comment 14 User image Lawrence Mandel [:lmandel] (use needinfo) 2015-07-09 10:33:13 PDT
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.
Comment 15 User image Bob Clary [:bc:] 2015-07-09 11:55:19 PDT
fyi, ran bughunter against the test case and only reproduced on beta/aurora.
Comment 16 User image yujitounai 2015-07-09 15:16:51 PDT
I'm not able to reproduce the crash with the latest Nightly.
It seems to be fixed.
Comment 17 User image Ritu Kothari (:ritu) 2015-07-10 11:45:42 PDT
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.
Comment 18 User image Ritu Kothari (:ritu) 2015-07-10 13:50:42 PDT
Comment on attachment 8629660 [details] [diff] [review]
crash fix

Discussed with Sylvestre, approving for uplift to Beta as well.
Comment 19 User image Ryan VanderMeulen [:RyanVM] 2015-07-13 06:54:02 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bea001f6780
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2015-07-13 09:10:41 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/447e511d00c7
Comment 21 User image Sylvestre Ledru [:sylvestre] 2015-07-15 16:59:27 PDT
Comment on attachment 8630050 [details] [diff] [review]
for esr38

Fix a crash, low risk, taking it.
Comment 22 User image Ryan VanderMeulen [:RyanVM] 2015-07-16 11:25:23 PDT
https://hg.mozilla.org/releases/mozilla-esr38/rev/b27fa5c1e8d9

Note You need to log in before you can comment on or make changes to this bug.