204623 - [FIX]Setting src attribute on img element created in a doc with no window crashes [@ nsImgManager::GetRootDocShell]

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Setting the src attribute (through setAttributeNS) on an img element (created
with createElementNS) not part of a document crashes. This looks like a
regression from bug 83774. This also affects XSLT transforms that create
elements, since the XSLT processor adds attributes to the elements before adding
them to the tree.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Testcase (WARNING: crashes)

Comment 2

[José Jeria]

Changing to all/all, also crashed on Windows 2000

Comment 3

[Peter Van der Beken [:peterv]]

Here's the relevant part of the stack:

 #0   0x0cdb9488 in nsImgManager::GetRootDocShell(nsIDOMWindow*, nsIDocShell**)
 #1   0x0cdb8850 in nsImgManager::ShouldLoad(int, nsIURI*, nsISupports*,
nsIDOMWindow*, int*)
 #2   0x10051c28 in nsContentPolicy::CheckPolicy(int, int, nsIURI*,
nsISupports*, nsIDOMWindow*, int*)
 #3   0x10051d50 in nsContentPolicy::ShouldLoad(int, nsIURI*, nsISupports*,
nsIDOMWindow*, int*)
 #4   0x10553950 in NS_CheckContentLoadPolicy(int, nsIURI*, nsISupports*,
nsIDOMWindow*, int*)
 #5   0x10114c84 in nsImageLoadingContent::CanLoadImage(nsIURI*, nsIDocument*)
 #6   0x1011410c in nsImageLoadingContent::ImageURIChanged(nsACString const&)
 #7   0x10113f1c in nsImageLoadingContent::ImageURIChanged(nsAString const&)
 #8   0x10182de4 in nsHTMLImageElement::SetAttr(int, nsIAtom*, nsAString const&,
int)
 #9   0x101495c4 in nsGenericHTMLElement::SetAttr(nsINodeInfo*, nsAString
const&, int)
 #10  0x10182e50 in nsHTMLImageElement::SetAttr(nsINodeInfo*, nsAString const&, int)
 #11  0x10093568 in nsGenericElement::SetAttributeNS(nsAString const&, nsAString
const&, nsAString const&)
 #12  0x10603328 in nsHTMLImageElement::SetAttributeNS(nsAString const&,
nsAString const&, nsAString const&)

Comment 4

[Peter Van der Beken [:peterv]]

It crashes on
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsImgManager.cpp#363
because aWindow is null.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well.... that document indeed has no window associated with it.  This is a
content policy bug -- it should null-check the window object it's passed, since
not all callers have a window object to give it (or better yet, we should take
dwitte's suggestion and eliminate the use of window objects in content policy
altogether).

This has come up before, and I fixed people to always create images in the right
document, but I forgot that we have this createDocument() business...

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the question is: would one expect to be able to access useful things about
that image (such as its width and height) after creating the <img> node like
that?  If so, we need to add a null-check to nsImgManager and have this open as
a privacy hole in mailnews (assuming someone has JS enabled in mailnews, of
course, in which case they're already screwed).  If not, we could just not load
the image when we have no window object... (we do this "by hand" for XML loaded
as data in the XML content sink).  But would that break XSLT?

Comment 7

[Peter Van der Beken [:peterv]]

Not sure, the case I was originally testing was transforming to a new document
and then setting that as the contentDocument of a bowser element. I never got to
verify whether that actually works, but if it does we need to load the images at
some point.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ugh. Yeah, if you're doing that then we just need the null-check and we need to
implement dwitte's system ASAP.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: nullcheck aWindow

The patch to nullcheck aWindow.

It is better to not rely too much on callers being nice :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple fix by being a good nsCOMPtr citizen

The CallQI change is not relevant to this bug but Had To Be Made.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Eep.  I totally missed mvl attaching a patch too.  :)  Well, r/sr=me on mvl's
patch if he prefers it to mine (mvl, you're basically the owner of this code as
far as I'm concerned, so what you say goes  :) )

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 122596 [details] [diff] [review]
Simple fix by being a good nsCOMPtr citizen

Well, we submitted them at almost the same time :)
I prefer this, as the docs on xpcom tell to not use QueryInterface directly.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 122596 [details] [diff] [review]
Simple fix by being a good nsCOMPtr citizen

Thanks for the cleanup, my finger started itching when I saw that code :-).

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 122596 [details] [diff] [review]
Simple fix by being a good nsCOMPtr citizen

requesting approval for 1.4b.
This is a simple fix for a crash, and is low-risk. This code is just following
the rules now.

Comment 15

[[e-mail]]

Comment on attachment 122596 [details] [diff] [review]
Simple fix by being a good nsCOMPtr citizen

clicking over to approval1.4? since 1.4b's release is imminent.

Comment 16

[[e-mail]]

Yeah, I can see definite advantages with this magical fix bz refers to in
comment 8. However, note that it's not my idea, it was mvl's. I'm just pushing
it. :)

I'll throw together a small doc on it tomorrow. Maybe we can remove the
channel/docshell/whatever inparam from contentpolicy API's completely because of
this?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, we would like to keep the channel (for ShouldProcess, not ShouldLoad, I
would say).  And we may want to keep an aContext that will be somehow usefully
defined, maybe.  But we definitely do not want to pass in a nsIDOMWindow.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 122596 [details] [diff] [review]
Simple fix by being a good nsCOMPtr citizen

a=asa (on behalf of drivers) for checkin to 1.4.

Comment 19

[[e-mail]]

checked in to trunk.