The default bug view has changed. See this FAQ.

XPConnect is passing null references into xpcom interfaces

RESOLVED FIXED in mozilla11

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bholley)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla11
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 577365 [details]
testcase (crashes Firefox when loaded) (requires extension for CC)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

(typeof window.getInterface) changes from "undefined" to "function" when the cycle collector runs.  And calling the function crashes [@ nsGlobalWindow::GetInterface].

Maybe a regression from recent cycle collector changes?
(Reporter)

Updated

5 years ago
Blocks: 326633
(Reporter)

Comment 1

5 years ago
Created attachment 577367 [details]
crash stack
I doubt this has anything to do with cycle collection, and everything to do with what fuzzPriv.CC does to get at dom window utils.
  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
  window.getInterface(null);

in an unprivileged context is enough to crash my build.
I'm going to guess this is fallout from Bug 692342.
Assignee: nobody → bobbyholley+bmo
Blocks: 692342
Component: DOM → XPConnect
OS: Mac OS X → All
QA Contact: general → xpconnect
Hardware: x86_64 → All
aIID is a null reference.
Summary: Crash with cycle collector, window.getInterface → XPConnect is passing null references into xpcom interfaces
(Reporter)

Comment 6

5 years ago
Created attachment 577782 [details]
testcase 2

Thanks for the reduction, Kyle.
Attachment #577365 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
Will the fix for bug 605271 (or new-dom-bindings) prevent this QI?
Yes, but even with that there's still an actual XPConnect bug here.
Created attachment 578305 [details] [diff] [review]
Bug 705875 - Check for null IID pointers and references in XPCConvert. v1

This is a regression from http://hg.mozilla.org/mozilla-central/rev/c428312abbc7 . The rest of the changes in that patch should be fine.

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=898f757866f3

When it goes green I'll flag for review. Hopefully we don't rely on passing null IIDs anywhere.
(In reply to Bobby Holley (:bholley) from comment #9)
> When it goes green I'll flag for review. Hopefully we don't rely on passing
> null IIDs anywhere.

We'd better not, because that's not valid C++!
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> (In reply to Bobby Holley (:bholley) from comment #9)
> > When it goes green I'll flag for review. Hopefully we don't rely on passing
> > null IIDs anywhere.
> 
> We'd better not, because that's not valid C++!

No, I mean in the case where we pass an IID pointer rather than an IID reference. I think we support both?
(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > (In reply to Bobby Holley (:bholley) from comment #9)
> > > When it goes green I'll flag for review. Hopefully we don't rely on passing
> > > null IIDs anywhere.
> > 
> > We'd better not, because that's not valid C++!
> 
> No, I mean in the case where we pass an IID pointer rather than an IID
> reference. I think we support both?

Oh, idk.
Comment on attachment 578305 [details] [diff] [review]
Bug 705875 - Check for null IID pointers and references in XPCConvert. v1

All the tests pass on linux, which is good enough for me.

Flagging khuey for review.
Attachment #578305 - Flags: review?(khuey)
Attachment #578305 - Flags: review?(khuey) → review+
This ended up with a bunch of oranges on mochitest-oth, which all seem to be related to some stuff already on inbound. But I rebased and am doing another try push (for mochitest-o only) to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=8a115e35ad03
Looks good. Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/59fe691e50d3
Flags: in-testsuite+
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/59fe691e50d3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.