785191 - Tons of "NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file content/base/src/nsImageLoadingContent.cpp" when sorting a file list

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit ftp://ftp.mozilla.org/pub/mozilla.org/artwork/ in a debug mozilla-central build

 2. Click "Name" or "Last Modified" to sort by those fields

ACTUAL RESULTS:
A bunch of spam like this to your console:
WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../mozilla/content/base/src/nsImageLoadingContent.cpp, line 153
WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../mozilla/content/base/src/nsImageLoadingContent.cpp, line 177
WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../mozilla/content/base/src/nsImageLoadingContent.cpp, line 188
WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../mozilla/content/base/src/nsImageLoadingContent.cpp, line 198
WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../mozilla/content/base/src/nsImageLoadingContent.cpp, line 208

EXPECTED RESULTS:
No spam.


This is from code like this:
150 NS_IMETHODIMP
151 nsImageLoadingContent::OnStartDecode(imgIRequest* aRequest)
152 {
153   NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE);
154 
155   LOOP_OVER_OBSERVERS(OnStartDecode(aRequest));
156   return NS_OK;
157 }

and we're failing that first line.

NOTE:  The spam seems to correspond to the file-icons in the list, and it does *not* seem to happen for folder-icons, for some reason.

So for example, if I repeat the STR at the URL ftp://ftp.mozilla.org/pub/mozilla.org/labs/weave/ , which just has folder-icons, I don't get any spam whatsoever -- whereas if I visit ftp://ftp.mozilla.org/pub/mozilla.org/data/ , which has folders and a single file icon, I get the spam.

We probably (?) want to just change these NS_ENSURE_TRUE methods to standard early-returns.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Uh.  What does the callstack look like there?

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: backtrace

Here's the backtrace, the first time IsCallerChrome returns false and triggers console-spam.

Comment 3

[Daniel Holbert [:dholbert]]

Reduced testcase:
data:text/html,Click:<img src="moz-icon://.html" onclick="this.parentNode.removeChild(this)">

Clicking the image (removing it from the DOM) triggers the warning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  that makes sense for OnDiscard.  What's the stack for OnStartDecode?

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: backtrace for OnStartDecode

Looks like it's just on BindToTree when the <img> element is (re)inserted.

And because the img is a special "moz-icon" image, we get an immediate decode, rather having it be async.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah.  So I think we should be pushing a null JSContext or whatever the equivalent is nowadays of saying "Pretend I'm not calling from script" around the AddImage call there.  And add a comment that says we're doing that so we don't look like we're calling our callbacks from JS.

For discard we should consider the same thing, actually.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> For discard we should consider the same thing, actually.

We should be waiting for the discard timer, not discarding synchronously.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Ah.  So I think we should be pushing a null JSContext or whatever the
> equivalent is nowadays of saying "Pretend I'm not calling from script"
> around the AddImage call there.
Looks like there's an example of that in nsImageLoadingContent::ClearPendingRequest:

1096   // Push a null JSContext on the stack so that code that runs within
1097   // the below code doesn't think it's being called by JS. See bug
1098   // 604262.
1099   nsCxPusher pusher;
1100   pusher.PushNull();

https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp#1090

So we just need to crib that code in BindToTree and UnbindFromTree (except it sounds like bz wants to make UnbindFromTree a little fancier, per comment 7).

Comment 9

[Daniel Holbert [:dholbert]]

I'll add the pusher.PushNull() stuff, to fix this bug, and then I'll file a followup for Comment 7, if that sounds OK.  (since it's conceptually separate, to a certain degree)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix

This fixes the STR for me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 654782 [details] [diff] [review]
fix

r=me

Comment 12

[Daniel Holbert [:dholbert]]

https://tbpl.mozilla.org/?tree=Try&rev=fa18001d0a76

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> (In reply to Boris Zbarsky (:bz) from comment #6)
> > For discard we should consider the same thing, actually.
> 
> We should be waiting for the discard timer, not discarding synchronously.

I agree with this.
Why are we discarding synchronously? We don't want discard when moving <img> around DOM.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (except it sounds like bz wants to make UnbindFromTree a little fancier, per comment
> 7).

er, s/bz/khuey/

I filed bug 785231 on the sync vs async discarding -- let's take that discussion over there.

Comment 15

[Daniel Holbert [:dholbert]]

Try has several completed all-green rows, so I think this is OK to push:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/6e67b20609d2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e67b20609d2