Closed Bug 1108721 Opened 5 years ago Closed 5 years ago

xul!mozilla::dom::WrapNewBindingObjectHelper<nsRefPtr<mozilla::dom::SpeechGrammarList>,1>::Wrap (NULL Pointer Dereference)

Categories

(Core :: DOM: Core & HTML, defect)

34 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox38 --- verified
firefox-esr31 36+ verified

People

(Reporter: vulnerable.zappa, Assigned: smaug)

References

Details

(4 keywords)

Crash Data

Attachments

(3 files)

Attached file ff33-all.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

1. Open ff33.html as local file (remote was not check)


Actual results:

Firefox crash


Expected results:

Nothing
Component: Untriaged → DOM
Product: Firefox → Core
This reproduces on Nightly as well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Crash Signature: [@ mozilla::dom::WrapNewBindingObjectHelper<nsRefPtr<nsSVGElement>, int>::Wrap(JSContext*, nsRefPtr<nsSVGElement> const&, JS::MutableHandle<JS::Value>)]
A null pointer where it shouldn't be, perhaps?
Per request on IRC... :-)
Flags: needinfo?(bzbarsky)
Attached file Testcase
Comment 4 is spot on:

WARNING: NS_ENSURE_TRUE(scriptObject || !hasHadScriptObject) failed: file ../../../mozilla/dom/html/TextTrackManager.cpp, line 98

So the TextTrackManager ctor never sets mTextTracks, which ends up null, and then HTMLMediaElement:::TextTracks() returns null but the IDL for it promises it will never do that so the bindings don't null-check.

Looks like a regression from bug 950308.
Blocks: 950308
Flags: needinfo?(rick.eyre)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Keywords: regression
Hmm, I guess we could use Document::GetScopeObject here.
Still a bit fragile, since GetScopeObject can in theory return null.
Assignee: nobody → bugs
Flags: needinfo?(bugs)
yeah, that is not enough. Need to make some things nullable in .webidl
Attached patch patchSplinter Review
I think we should do this for now.

Making GetScopeObject() to never return null if it has once returned non-null would be way more risky change, not anything for branches.
Attachment #8545315 - Flags: review?(peterv)
Comment on attachment 8545315 [details] [diff] [review]
patch

Review of attachment 8545315 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit torn on whether this should actually throw or return null, but this is less riskier. Eventually we should be able to undo this, hopefully we will remember to when GetParentObject never returns null on nodes.
Attachment #8545315 - Flags: review?(peterv) → review+
Comment on attachment 8545315 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Crashes
Fix Landed on Version: 37
Risk to taking this patch (and alternatives if risky): Shouldn't be risky 
String or UUID changes made by this patch:  NA

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: 
[User impact if declined]: bug 950308
[Describe test coverage new/current, TBPL]: the feature itself is tested by several mochitests, but this crash corner case indeed would require a crashtest.
[Risks and why]: Shouldn't be risky, effectively a null check
[String/UUID change made/needed]: NA
Attachment #8545315 - Flags: approval-mozilla-esr31?
Attachment #8545315 - Flags: approval-mozilla-beta?
Attachment #8545315 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fc33533af56c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8545315 [details] [diff] [review]
patch

aurora is 37, beta is 36. So, aurora is already fixed.
Attachment #8545315 - Flags: approval-mozilla-beta?
Attachment #8545315 - Flags: approval-mozilla-beta+
Attachment #8545315 - Flags: approval-mozilla-aurora?
Attachment #8545315 - Flags: approval-mozilla-aurora-
Sheriff, the patch should apply to branches with fuzz and the usual dom/foo -> content/bar rename.
Is there a bug already about making GetScopeObject not return null?
Flags: needinfo?(bugs)
I thought I had filed that, but apparently not.
Bug 1122041.
Flags: needinfo?(bugs)
Attachment #8545315 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
I was able to reproduce the issue in Firefox 34.0.5 using scenario from comment 0 on Windows 7 x64. Firefox no longer crashes in the same scenario in Firefox 36 Beta 8, latest Aurora and latest Nightly.
Status: RESOLVED → VERIFIED
Reproduced the original issue using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/34.0.5/win32/en-US/
** https://crash-stats.mozilla.com/report/index/36a1ae14-8489-48bb-8be5-61e062150218

Went through verification using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.5.0esr-candidates/build2/

Test Cases:

* Opened the POC from comment #0 several different times in normal tabs/windows
* Opened the POC from comment #0 several different times in private tabs/windows
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.