16543 - Must not use |QueryInterface()| as a factory

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Scott Collins]

Can't use |QueryInterface()| as a factory, that violates [XP]COMs rules about
always returning the same pointer for a given query against a given object.  This
will end up breaking XPConnect (and anything that makes the same basic
assumptions) and causing asserts in |nsCOMPtr|s type checks.  This is not the
same thing as aggregation, where the new object created in |QueryInterface()|
would be saved and returned again next time.  See

  <http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsRangeList.cpp#1447>
  <http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsRegionMac.cpp#131>

for two cases that I've caught so far.  I expect to find others.

Comment 1

[Suresh Duddi (gone)]

Gosh! gosh...

I sincerely hope these be the exception.

Comment 2

[Scott Collins]

It's not dogfood, I think, since no user-visible bad behavior can be attributed
to it, but it's likely to cause bugs and needs to be fixed soon.  Marking for
M12.

Comment 3

[Patrick C. Beard]

I fixed the second case in nsRegionMac.cpp.

Comment 4

[vidur (gone)]

Cc:ing Joe Francis - he originally wrote nsRangeList.cpp.

Comment 5

[Joe Francis]

nope.  I wrote most of nsRange.cpp, but not nsRangeList.cpp.  That is the
selection implementation, and is owned by mjudge.  cc'ing him instead.

Comment 6

[Scott Collins]

Not dogfood.

Comment 7

[Scott Collins]

As far as mjudge and I can tell, no callers remain who |QueryInterface| for an
enumerator in |nsRangeList|.  All callers have been fixed to call |GetEnumerator|
instead.  Just in case we missed one, however, we have not totally deleted the QI
clause for enumerator types.  We replaced it with an assert so we can catch and
fix any stragglers.

Comment 8

[Bruce Robson]

I've hit the assert (see 18th Jan 2000 comment) in a mozilla built from the 19th 
Jan tar ball. I'm running on NT.

I was attempting to copy the text "Workplace, health, safety and welfare 
IND(G)244L" out of page http://www.hse.gov.uk/press/e98175.htm

Comment 9

[Bruce Robson]

Attachment: stack trace

Comment 10

[Suresh Duddi (gone)]

Moving to M14 from M13.

Comment 11

[Scott Collins]

Looking at the source code, the culprit indicated by your stack trace is using 
the correct code (see line 606 of <http://lxr.mozilla.org/mozilla/source/layout/
base/src/nsGenericDOMDataNode.cpp>) checked in in version 3.32 on the 19th.  I 
suspect that this was checked in after the verification on that day... and the 
tar-balls were made from the verified pull, so the Jan 19 tar-ball just missed 
this fix by inches.

Comment 12

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/b4e965131896
[fenix] Nimbus Global Opt Out (https://github.com/mozilla-mobile/fenix/pull/16543) r=gl