603706 - Need a way to track the originating window for all nsIConsoleMessages

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

The Web Console currently uses the nsIConsoleService to listen for error messages (nsIScriptError) and general messages (nsIConsoleMessage). However, many of these messages come without any sourceName (which is obvious in the case of nsIConsoleMessages), and those that have the sourceName (nsIScriptErrors) point to scripts themselves.

Multiple tabs can have the same URI open, or a different URI that includes some of the same scripts, and this prevents us from properly picking the tab into which we have to display the messages.

We need an improved nsIConsoleService API that gives us the ability to track the originating window for any nsIConsoleMessage.

Comment 1

[Mihai Sucan [:msucan]]

CC'ing people as suggested by David.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think what we should do here is to add an nsIScriptError2 interface which contains a window property, and then make the nsContentUtils helper methods optionally take an nsIDocument argument and set the window property as appropriate.

Comment 4

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

(In reply to comment #2)
> I think what we should do here is to add an nsIScriptError2 interface which
> contains a window property

Bug 605492 essentially did that. Do we actually need this for all nsIConsoleMessages, rather than all nsIScriptErrors?

Comment 5

[Boris Zbarsky [:bzbarsky]]

There are probably cases where we use nsIConsoleMessage and know the window we belong to...

Comment 6

[Kevin Dangoor]

Given the work going on in other bugs, is this one still relevant?

Comment 7

[Mihai Sucan [:msucan]]

This bug will no longer be relevant when the other bug patches will land. Additionally, we'll tackle existing uses of nsIConsoleMessages on a case-by-case basis, and switch them to nsIScriptError2 which seems to work out just fine for our needs in the Web Console.

Comment 8

[Mihai Sucan [:msucan]]

Having looked through the Gecko code that makes use of nsIConsoleService, I found the following uses of nsIConsoleMessage:

nsJAR::ReportError()
http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJAR.cpp#842
.jar certificate errors

Decoder::Finish()
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/Decoder.cpp#105
image loading error: it reports image corruption

nsChromeRegistry::LogMessage()
http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeRegistry.cpp#82
chrome registry errors

OSMesaLibrary::EnsureInitialized() and LogMessage()
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/GLContextProviderOSMesa.cpp#62
OSMesa library initialization errors (software rendering for WebGL)

LayerManagerOGL::Initialize()
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#149
info message about successful OpenGL LayerManager initialization

DeviceManagerD3D9::Init()
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#198
info message about successful Direct3D 9 DeviceManager initialization

Nv3DUtils
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/Nv3DVUtils.cpp
messages about successful or failed initialization of Nv3DUtils

CCRunnableFaultReport and Fault()
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#1058
nsCycleCollector faults

LogConsoleMessage() and nsCommandLine::EnumerateHandlers()
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/src/nsCommandLine.cpp#576
failure to initialize a command line handler based on contract ID

txMessage::execute()
http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txInstructions.cpp#563
xsl message (what is it?)

nsHTMLDocument::GetSelection()
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#2524
warning about the use of a deprecated method: document.getSelection()

CanvasUtils::LogMessage() and CanvasUtils::LogMessagef()
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasUtils.cpp#101
both seem to be unused?

WebGLContext::LogMessage()
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextUtils.cpp#214
uses JS_ReportWarning()

WebGLContext::SetDimensions()
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#269
messages about the GL context initialization

WebGLTexture::NeedFakeBlack()
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.h#984
texture-related messages are logged

nsXULContentUtils::LogTemplateError()
http://mxr.mozilla.org/mozilla-central/source/content/xul/templates/src/nsXULContentUtils.cpp#516
reports parsing error messages about XUL Templates (what are XUL templates?)

nsXULTemplateBuilder::OutputMatchToLog()
http://mxr.mozilla.org/mozilla-central/source/content/xul/templates/src/nsXULTemplateBuilder.cpp#2561
logs XUL Template query match

HandshakeCallback() in nsNSSCallbacks.cpp
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#851
warns if the server does not support RFC 5746

nsWebSocketEstablishedConnection::PrintErrorOnConsole()
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#1934
logs WebSocket connection errors (see bug 603706)

Instead of adding a window ID to all of the nsIConsoleMessages, I think it's better we switch the code to use nsIScriptError2.

Which of the above cases do you guys think we should be able to display in the Web Console?

Comment 9

[David Dahl :ddahl]

(In reply to comment #8) 
Messages that emanate from content are all we care about right now. I am happy we have this history in the bug though. 

> nsJAR::ReportError()
no - chrome

> Decoder::Finish()
> image loading error: it reports image corruption
yes, good one.
> 
> nsChromeRegistry::LogMessage()
> chrome registry errors
no
> 
> OSMesaLibrary::EnsureInitialized() and LogMessage()
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/GLContextProviderOSMesa.cpp#62
> OSMesa library initialization errors (software rendering for WebGL)
yes, if it impacts content - does it?

> 
> LayerManagerOGL::Initialize()
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#149
> info message about successful OpenGL LayerManager initialization
not sure.

> 
> DeviceManagerD3D9::Init()
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#198
> info message about successful Direct3D 9 DeviceManager initialization
not sure

> 
> Nv3DUtils
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/Nv3DVUtils.cpp
> messages about successful or failed initialization of Nv3DUtils
> 
not sure

> CCRunnableFaultReport and Fault()
> http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#1058
> nsCycleCollector faults
no - not sure how exposing this would help content devs - maybe it would?

> 
> LogConsoleMessage() and nsCommandLine::EnumerateHandlers()
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/src/nsCommandLine.cpp#576
> failure to initialize a command line handler based on contract ID
no.

> 
> txMessage::execute()
> http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txInstructions.cpp#563
> xsl message (what is it?)
not sure

> 
> nsHTMLDocument::GetSelection()
> http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#2524
> warning about the use of a deprecated method: document.getSelection()
yes

> 
> CanvasUtils::LogMessage() and CanvasUtils::LogMessagef()
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasUtils.cpp#101
> both seem to be unused?
if unused, then no

> 
> WebGLContext::LogMessage()
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextUtils.cpp#214
> uses JS_ReportWarning()
yes

> 
> WebGLContext::SetDimensions()
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#269
> messages about the GL context initialization
yes

> 
> WebGLTexture::NeedFakeBlack()
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.h#984
> texture-related messages are logged
yes?

> 
> nsXULContentUtils::LogTemplateError()
> http://mxr.mozilla.org/mozilla-central/source/content/xul/templates/src/nsXULContentUtils.cpp#516
> reports parsing error messages about XUL Templates (what are XUL templates?)
no - XUL templates build dynamic XUL nodes from RDF or Sqlite

> 
> nsXULTemplateBuilder::OutputMatchToLog()
> http://mxr.mozilla.org/mozilla-central/source/content/xul/templates/src/nsXULTemplateBuilder.cpp#2561
> logs XUL Template query match
no

> 
> HandshakeCallback() in nsNSSCallbacks.cpp
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#851
> warns if the server does not support RFC 5746
yes

> 
> nsWebSocketEstablishedConnection::PrintErrorOnConsole()
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#1934
> logs WebSocket connection errors (see bug 603706)
i think yes

> 
> Instead of adding a window ID to all of the nsIConsoleMessages, I think it's
> better we switch the code to use nsIScriptError2.
I thought that was the plan, no?

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #9)
> (In reply to comment #8) 
> Messages that emanate from content are all we care about right now. I am happy
> we have this history in the bug though. 

Yup, that's why I posted this list here. Thanks for your reply!


> > OSMesaLibrary::EnsureInitialized() and LogMessage()
> > http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/GLContextProviderOSMesa.cpp#62
> > OSMesa library initialization errors (software rendering for WebGL)
> yes, if it impacts content - does it?

I think it does. In the sense that, if you have a GL context that fails to initialize, you want to know. I doubt a content web dev can do anything about the error, if it happens, but at least he sees something went wrong.


> > LayerManagerOGL::Initialize()
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#149
> > info message about successful OpenGL LayerManager initialization
> not sure.
> 
> > 
> > DeviceManagerD3D9::Init()
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#198
> > info message about successful Direct3D 9 DeviceManager initialization
> not sure
> 
> > 
> > Nv3DUtils
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/Nv3DVUtils.cpp
> > messages about successful or failed initialization of Nv3DUtils
> > 
> not sure

Same as above.

> > CCRunnableFaultReport and Fault()
> > http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#1058
> > nsCycleCollector faults
> no - not sure how exposing this would help content devs - maybe it would?

Seems kind of "debugish" info. I'd say no.


> > CanvasUtils::LogMessage() and CanvasUtils::LogMessagef()
> > http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasUtils.cpp#101
> > both seem to be unused?
> if unused, then no

I looked through mxr, no results.


> > WebGLContext::LogMessage()
> > http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextUtils.cpp#214
> > uses JS_ReportWarning()
> yes

This should work once bug 597136 lands.


> > nsXULContentUtils::LogTemplateError()
> > http://mxr.mozilla.org/mozilla-central/source/content/xul/templates/src/nsXULContentUtils.cpp#516
> > reports parsing error messages about XUL Templates (what are XUL templates?)
> no - XUL templates build dynamic XUL nodes from RDF or Sqlite

Thanks! I didn't know you can do that.


> > Instead of adding a window ID to all of the nsIConsoleMessages, I think it's
> > better we switch the code to use nsIScriptError2.
> I thought that was the plan, no?

Yes.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do you want to use nsIScriptError for things that don't have a filename/lineno?

Comment 12

[Mihai Sucan [:msucan]]

(In reply to comment #11)
> Do you want to use nsIScriptError for things that don't have a filename/lineno?

I've seen nsIScriptError used for things that have no line number. I wouldn't use it without a file name / location.

If I can't find a file name, then I'll not really be able to make any change. If I get the window ID then I can also get the location.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This patch unearths:

- the document.getSelection() warning about the method being deprecated,
- and raster image corruption errors.

WebGL errors will all show once bug 597136 lands. No need to make changes here.

nsWebSocket errors will show once bug 603750 lands.

LayerManager and OSMesa-related messages do not need to show up in the Web Console. After closer inspection, those are chrome errors/warnings, not related to web pages.

The HandshakeCallback() warning from nsNSSCallbacks.cpp seems to be too deep into the Gecko codebase. It's not trivial to find a window ID and a document location from there. If someone has ideas, please let me know.

Looking forward to the review. Thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

The imagelib bits here certainly need review from an imagelib peer.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 490116 [details] [diff] [review]
proposed patch

Asking for review from an imagelib peer as well. Thanks Boris!

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 490116 [details] [diff] [review]
proposed patch

I'm mostly out of the loop 'til summer - switching r? to joe.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 490116 [details] [diff] [review]
proposed patch

>diff --git a/modules/libpr0n/src/Decoder.cpp b/modules/libpr0n/src/Decoder.cpp

>@@ -112,21 +113,35 @@ Decoder::Finish()

>+      consoleService->LogMessage(
>+        (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));
>     }

For greater code cleanliness, can you declare a temporary nsCOMPtr<nsIScriptError> and then pass that to LogMessage?

>diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h

>+  void SetWindowID(PRUint64 aWindowId) {
>+    mWindowId = aWindowId;
>+  }
>+  PRUint64 WindowID() { return mWindowId; }

>+  PRUint64 mWindowId;

I'd much rather see these as an attribute on mozilla::imagelib::Image, actually; there's no reason why VectorImages wouldn't need their window ID to report errors too, if they ever want to.

>diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp

>@@ -1640,16 +1642,25 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
>     // is redirected, we'll have a way to cancel the resulting channel.
>     nsCOMPtr<nsILoadGroup> loadGroup =
>         do_CreateInstance(NS_LOADGROUP_CONTRACTID);
>     newChannel->SetLoadGroup(loadGroup);
> 
>     void *cacheId = NS_GetCurrentThread();
>     request->Init(aURI, aURI, loadGroup, newChannel, entry, cacheId, aCX);
> 
>+    // Pass the windowID of the loading document, if possible.
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aCX);
>+    if (doc) {
>+      nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
>+      if (win) {
>+        request->SetWindowID(win->WindowID());
>+      }
>+    }

This is sort of scary, because aCX is just a void*, and we're using the current implementation to get this pretty critical data. I'd much prefer the window ID be sent in as another parameter to loadImage, even though that means more code churn.

Also, how well does nsIScriptError2::InitWithWindowID() handle window IDs of zero?

>diff --git a/modules/libpr0n/src/imgRequest.h b/modules/libpr0n/src/imgRequest.h

>   nsCOMPtr<nsIChannel> mNewRedirectChannel;
>   // Sometimes consumers want to do things before the image is ready. Let them,
>   // and apply the action when the image becomes available.
>   PRPackedBool mDecodeRequested : 1;
> 
>   PRPackedBool mIsMultiPartChannel : 1;
>   PRPackedBool mGotData : 1;
>   PRPackedBool mIsInCache : 1;
>+
>+  PRUint64 mWindowId;

This should be a) documented and b) moved to above the PRPackedBools to avoid packing/alignment overhead.

Minusing so I can get another look at this after the changes.

Comment 18

[Mihai Sucan [:msucan]]

(In reply to comment #17)
> Comment on attachment 490116 [details] [diff] [review]
> proposed patch
> 
> >diff --git a/modules/libpr0n/src/Decoder.cpp b/modules/libpr0n/src/Decoder.cpp
> 
> >@@ -112,21 +113,35 @@ Decoder::Finish()
> 
> >+      consoleService->LogMessage(
> >+        (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));
> >     }
> 
> For greater code cleanliness, can you declare a temporary
> nsCOMPtr<nsIScriptError> and then pass that to LogMessage?

Sure.


> >diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h
> 
> >+  void SetWindowID(PRUint64 aWindowId) {
> >+    mWindowId = aWindowId;
> >+  }
> >+  PRUint64 WindowID() { return mWindowId; }
> 
> >+  PRUint64 mWindowId;
> 
> I'd much rather see these as an attribute on mozilla::imagelib::Image,
> actually; there's no reason why VectorImages wouldn't need their window ID to
> report errors too, if they ever want to.

I can do this, I wasn't sure that's preferred.


> >diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp
> 
> >@@ -1640,16 +1642,25 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
> >     // is redirected, we'll have a way to cancel the resulting channel.
> >     nsCOMPtr<nsILoadGroup> loadGroup =
> >         do_CreateInstance(NS_LOADGROUP_CONTRACTID);
> >     newChannel->SetLoadGroup(loadGroup);
> > 
> >     void *cacheId = NS_GetCurrentThread();
> >     request->Init(aURI, aURI, loadGroup, newChannel, entry, cacheId, aCX);
> > 
> >+    // Pass the windowID of the loading document, if possible.
> >+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aCX);
> >+    if (doc) {
> >+      nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
> >+      if (win) {
> >+        request->SetWindowID(win->WindowID());
> >+      }
> >+    }
> 
> This is sort of scary, because aCX is just a void*, and we're using the current
> implementation to get this pretty critical data. I'd much prefer the window ID
> be sent in as another parameter to loadImage, even though that means more code
> churn.

aCX is nsISupports* according to the LoadImage() signature. Afaik, nsISupports is exactly what I needed: something to QI to what I need. Am I wrong? Please let me know what change you'd like me to do. (I am new to Gecko!)


> Also, how well does nsIScriptError2::InitWithWindowID() handle window IDs of
> zero?

No problem with window IDs of 0.


> >diff --git a/modules/libpr0n/src/imgRequest.h b/modules/libpr0n/src/imgRequest.h
> 
> >   nsCOMPtr<nsIChannel> mNewRedirectChannel;
> >   // Sometimes consumers want to do things before the image is ready. Let them,
> >   // and apply the action when the image becomes available.
> >   PRPackedBool mDecodeRequested : 1;
> > 
> >   PRPackedBool mIsMultiPartChannel : 1;
> >   PRPackedBool mGotData : 1;
> >   PRPackedBool mIsInCache : 1;
> >+
> >+  PRUint64 mWindowId;
> 
> This should be a) documented and b) moved to above the PRPackedBools to avoid
> packing/alignment overhead.

Done.


> Minusing so I can get another look at this after the changes.


Thanks a lot for your review! Will update the patch.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch, based on Joe's review.

Comment 20

[Mihai Sucan [:msucan]]

Also, wouldn't changing LoadImage()'s signature (to add a new argument for window ID) mean I should update the imgILoader IDL? This is a public interface, and the API is frozen since fx4b7. This is a reason why I stayed away making such code changes.

Comment 21

[Joe Drew (not getting mail)]

(In reply to comment #18)

> aCX is nsISupports* according to the LoadImage() signature. Afaik, nsISupports
> is exactly what I needed: something to QI to what I need. Am I wrong? Please
> let me know what change you'd like me to do. (I am new to Gecko!)

Huh! I must have misremembered. This is fine.

Comment 22

[Joe Drew (not getting mail)]

Comment on attachment 491592 [details] [diff] [review]
updated patch

Can you also add a comment to mWindowID in Image.h?

Looks great otherwise. Thanks!

Comment 23

[Mihai Sucan [:msucan]]

Thanks Joe for the r+!

Asking for blocking2.0+. This patch makes a number of warnings and errors to show up in the Web Console. Once bug 611789 lands, this patch is one of the important ones to ensure we are not missing messages.

Comment 24

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

"for all nsIConsoleMessages" is a bit vague for blocking purposes - is this just meant to cover the instances in that patch specifically? IIRC there is another bug about making similar changes. Let's just avoid expanding this bug's patch scope, and use new bugs if needed.

Comment 25

[Mihai Sucan [:msucan]]

(In reply to comment #24)
> "for all nsIConsoleMessages" is a bit vague for blocking purposes - is this
> just meant to cover the instances in that patch specifically? IIRC there is
> another bug about making similar changes. Let's just avoid expanding this bug's
> patch scope, and use new bugs if needed.

You are correct. It started as a bug about nsIConsoleMessages, but then Boris was kind of enough to provide us with a new nsIScriptError2 which suffices our needs - in bug 605492. Then we've opened up bug 606498 to switch wherever possible from nsIScriptError to nsIScriptError2, in the Gecko codebase.

There's bug 603750 which specifically deals with the nsWebSocket connection failure messages.

And finally, this bug deals with the remaining batch of errors/warnings that do not use nsIScriptError at all - just basic nsIConsoleMessages. This patch changes them to use nsIScriptError2, where possible.

I will not expand the purpose of this patch, or of the others.

Thanks for the blocking2.0+!

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491592 [details] [diff] [review]
updated patch

r=me, though I wonder whether we could have gotten the document from the loadgroup lazily as needed...

Also, there was talk of having a helper function for getting window id from a document, right?

Comment 27

[Mihai Sucan [:msucan]]

(In reply to comment #26)
> Comment on attachment 491592 [details] [diff] [review]
> updated patch
> 
> r=me, though I wonder whether we could have gotten the document from the
> loadgroup lazily as needed...

Not sure how I can do that, but I investigated the code and... my finding is that we could perhaps not touch imgLoader. We could probably get away with only touching imgRequest::OnDataAvailable() if ctxt (nsISupports) is the same aCX from imgLoader::LoadImage(). Is that the case? If yes, then I can move the code which does QI to nsIDocument from imgLoader to imgRequest. Do you want me to do this change? It would simplify things a bit.

The image Decoder does not have access to the originating request, load group, or loader.


> Also, there was talk of having a helper function for getting window id from a
> document, right?

Yes, we have that in bug 606498. Will attach an updated patch which depends on that code.

Thanks for your review+!

Comment 28

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated the patch to address your review: made it to use the new nsIDocument->OuterWindowID() helper method from the patches of bug 606498. I also made Image::WindowID() and imgRequest::WindowID() to be const methods, because they do not change anything when they are invoked.

With regards to my previous comment: I also tested using the ctxt parameter in imgRequest::OnDataAvailable() without any imgLoader changes: no luck. ctxt doesn't seem to be able to QI to nsIDocument.

If you have a specific idea on how to this in a better way, please let me know and I'll update the code accordingly. Thanks!

Comment 29

[Boris Zbarsky [:bzbarsky]]

> The image Decoder does not have access to the originating request,
> load group, or loader.

Hmm.  So it doesn't have an imgRequest, I guess?  I guess those aren't in the right loadgroup anyway, so don't worry about it too much.  Note that we coalesce image loads across documents (including across totally unrelated tabs, etc), so you'll only get the error report in the first window that started the image load.  That might be ok for your purposes, though.

Comment 30

[Joe Drew (not getting mail)]

Decoders do have access to their imgRequest through mObserver, but they'd have to QI to it.

Comment 31

[Mihai Sucan [:msucan]]

(In reply to comment #29)
> > The image Decoder does not have access to the originating request,
> > load group, or loader.
> 
> Hmm.  So it doesn't have an imgRequest, I guess?  I guess those aren't in the
> right loadgroup anyway, so don't worry about it too much.  Note that we
> coalesce image loads across documents (including across totally unrelated tabs,
> etc), so you'll only get the error report in the first window that started the
> image load.  That might be ok for your purposes, though.

I was aware of that coalescing. It's fine for your purposes.


(In reply to comment #30)
> Decoders do have access to their imgRequest through mObserver, but they'd have
> to QI to it.

Oh, I haven't noticed that. Still, from the Decoder can we use the imgRequest to access the context (nsIDocument) of the imgLoader?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> can we use the imgRequest to access the context (nsIDocument) of the imgLoader?

Before the end of the load (so while it still has an nsIChannel), yes.

Comment 33

[Mihai Sucan [:msucan]]

Attachment: [checkin] rebased patch

Rebased the patch on top of the latest changes in bug 606498, bug 603750 and mozilla-central trunk. Ready for checkin.

Comment 34

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 498501 [details] [diff] [review]
[checkin] rebased patch

http://hg.mozilla.org/mozilla-central/rev/4e6f930a1363