373393 - ASSERTION: Lying nsIInterfaceRequestor implementation! (mozilla/content/base/src/nsXMLHttpRequest.cpp)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Nickolay_Ponomarev]

When the update happens (usually when you leave the build working for a while), the following assertion happens:

"###!!! ASSERTION: Lying nsIInterfaceRequestor implementation!: '*aResult', file s:/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 2080"

The relevant code is:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.174#2077

And the nsIInterfaceRequestor impl in question is BadCertHandler:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/shared/src/badCertHandler.js&rev=1.1&mark=104-105#99

The reason this assertion happens is bug 287107 (Components.returnCode being ignored), but since that bug didn't receive much action, we might as well fix the getInterface fn to throw instead of relying on returnCode. (Note bug 243621, but there wasn't an agreement about it and not asserting is better than being less annoying in Venkman), just because fewer people use Venkman than the debug build.

Comment 1

[Nickolay_Ponomarev]

Attachment: patch

switch to |throw|

Comment 2

[Alex Vincent [:WeirdAl]]

You'd best leave the return null line in there, even if it never gets called.  Otherwise, Mozilla will report a strict warning.

Comment 3

[Nickolay_Ponomarev]

Attachment: patch v2

gavin convinced me this can just call QI. And there's another GI implementation in nsUpdateService.js

As for |return|, I don't think you're right, but if it was the case, it would be a bug in the engine and would need to be fixed instead of making perfectly good code uglier.

Comment 4

[(not reading, please use [e-mail] instead)]

because the change is badCertHandler.js, cc dveditz

Comment 5

[Jesse Ruderman]

Dup of bug 372482?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd rather you didn't make GI call QI... the two are conceptually different, and this makes it harder to see what interfaces are actually obtainable using GetInterface.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> I'd rather you didn't make GI call QI... the two are conceptually different,
> and this makes it harder to see what interfaces are actually obtainable using
> GetInterface.

All of the interfaces that this object implement are "obtainable" using GetInterface, so I don't understand what you mean. I understand the conceptual difference between QI and GI, but there's no practical difference in this case, so I don't see why we should duplicate code unnecessarily.

Comment 9

[Nickolay_Ponomarev]

biesi: that was exactly my position and the way gavin convinced me calling QI from GI is ok is comment 8 and the fact that the rest of simple GI implementations in browser/ and toolkit/ do this already.

At least I didn't use sharp variables :)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

well, ok... leave it as-is then, I guess...

Comment 11

[Nickolay_Ponomarev]

mozilla/toolkit/mozapps/shared/src/badCertHandler.js      1.2
mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in  1.130

Comment 12

[Jesse Ruderman]

I still see this assertion, along with the extension manager bugginess, when I switch from a nightly to an up-to-date debug build.

Comment 13

[Nickolay_Ponomarev]

When testing, you have to clobber toolkit/mozapps to make sure nsUpdateService gets rebuilt and the change to badcerthandler is included in the actual JS component file.

Comment 14

[Jesse Ruderman]

After clobbering my entire objdir, I no longer get two "Lying nsIInterfaceRequestor" assertions.  Instead, I get five of these when switching builds:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Component does not have requested interface' when calling method: [nsIInterfaceRequestor::getInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: file:///Users/jruderman/trunk/mozilla/fx-debug-shared-gcc401-cc/dist/MinefieldDebug.app/Contents/MacOS/components/nsExtensionManager.js :: anonymous :: line 3715"  data: no]
************************************************************

... and extension manager is still broken when switch builds. 

Comment 15

[Nickolay_Ponomarev]

(In reply to comment #14)
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'Component does not have requested interface' when calling
> method: [nsIInterfaceRequestor::getInterface]"  nsresult: "0x80004002
> (NS_NOINTERFACE)"  location: "JS frame ::
> file:///Users/jruderman/trunk/mozilla/fx-debug-shared-gcc401-cc/dist/MinefieldDebug.app/Contents/MacOS/components/nsExtensionManager.js
> :: anonymous :: line 3715"  data: no]
> ************************************************************
> 
Well, indeed "Call to xpconnect wrapped JSObject produced an error" (exception). Not sure why it needs to be printed in the console though, esp. in the case of the getInterface call.

> ... and extension manager is still broken when switch builds. 
> 
Not sure how this is related to this bug.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> I'd rather you didn't make GI call QI... the two are conceptually different,
> and this makes it harder to see what interfaces are actually obtainable using
> GetInterface.

For what it's worth, I agree with biesi here.

GetInterface is essentially a cheap (and ugly) way of adding getters.  In other words, rather than adding |readonly attribute nsIBar bar| to |nsIFoo|, you make a rule that you can, on an nsIFoo, do foo.QueryInterface(nsIInterfaceRequestor IID).getInterface(nsIBar IID) to do the same thing.  Implementing getInterface where no known semantics for getInterface exist is like implementing an interface method that doesn't exist (and, somewhat dangerously, might exist in the future).

Comment 17

[Nickolay_Ponomarev]

I understand they are different. You have to explain this to the people who wrote the rest of getInterface implementations in browser/ and toolkit/. I'm trying to be consistent with the rest of the code, since in these specific cases it's not harmful to implement getInterface the way it's done, IMO.

If browser/toolkit folks and you can reach an agreement, I'll be happy to switch all the implementations to the chosen way of implementing QI/GI.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> Implementing getInterface where no known semantics for getInterface exist is
> like implementing an interface method that doesn't exist (and, somewhat
> dangerously, might exist in the future).

We're constrained by the nsIChannel API in this case. The channel takes an nsIInterfaceRequestor and uses it to get to request nsIProgressEventSink, nsIPrompt, nsIAuthPrompt, among others (the list isn't well defined). I'm not sure why it doesn't instead take an nsISupports and use QI, but presumably it's because some of the requested interfaces might conflict or can't be implemented by a single object. Either way, we need to implement nsIInterfaceRequestor. In this specific case (and the others that use similar code), the two implementations (QI and GI) would only trivially differ, so instead of duplicating the code we just simplified it by calling QI. If the only problem with this approach is that it might confuse others who don't understand the semantic differences between QI and GI, or that it might cause problems in the future if these interfaces and implementations change, I think the tradeoffs are worth it (I think the chances of the relationship between these objects changing in a way that could be problematic are pretty low).

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(didn't realize you weren't CCed...)

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Since you asked why Necko does it this way...

In many cases, the interfaces are implemented by different objects. For example, in many cases, the object used for nsIAuthPrompt/nsIPrompt is the one gotten from the windowwatcher (getNewPrompter/getNewAuthPrompter).

On the other hand, things like nsIProgressEventSink may well be implemented by the streamlistener object.

Since QueryInterface semantics wouldn't really allow that, getInterface simplifies implementing this interface for cases like that.