Closed
Bug 1180389
Opened 9 years ago
Closed 9 years ago
Crash [@ nsJSChannel::SetLoadInfo(nsILoadInfo*) ] | Firefox 39.0 crashes immediately by javascript
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
People
(Reporter: bogus, Assigned: smaug)
References
Details
(Keywords: crash, csectype-nullptr, testcase)
Crash Data
Attachments
(4 files)
108 bytes,
text/html
|
Details | |
131.29 KB,
text/plain
|
Details | |
1.18 KB,
patch
|
bzbarsky
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
bzbarsky
:
review+
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
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•9 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•9 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
Updated•9 years ago
|
Keywords: csectype-nullptr,
testcase
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Oh, and we may want to backport to ESR38, but fix the maybe-leak while we're there....
Flags: needinfo?(bugs)
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
Flags: needinfo?(bugs)
Attachment #8630050 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•9 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•9 years ago
|
||
Comment on attachment 8630050 [details] [diff] [review] for esr38 r=me
Attachment #8630050 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•9 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?
Comment 12•9 years ago
|
||
Setting nom for ESR38 tracking until the 40+ option is added to the list.
status-firefox-esr38:
--- → affected
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
tracking-firefox-esr38:
--- → ?
https://hg.mozilla.org/mozilla-central/rev/b09937a4c395
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•9 years ago
|
Comment 14•9 years ago
|
||
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•9 years ago
|
||
fyi, ran bughunter against the test case and only reproduced on beta/aurora.
Reporter | ||
Comment 16•9 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+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bea001f6780
Flags: in-testsuite+
Comment 21•9 years ago
|
||
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.
Description
•