Last Comment Bug 785191 - Tons of "NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file content/base/src/nsImageLoadingContent.cpp" when sorting a file list
: Tons of "NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file conten...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 12:40 PDT by Daniel Holbert [:dholbert]
Modified: 2012-08-24 20:06 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
backtrace (14.28 KB, text/plain)
2012-08-23 13:14 PDT, Daniel Holbert [:dholbert]
no flags Details
backtrace for OnStartDecode (16.68 KB, text/plain)
2012-08-23 13:39 PDT, Daniel Holbert [:dholbert]
no flags Details
fix (1.84 KB, patch)
2012-08-23 13:59 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2012-08-23 12:40:46 PDT
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 [:bz] 2012-08-23 12:46:12 PDT
Uh.  What does the callstack look like there?
Comment 2 Daniel Holbert [:dholbert] 2012-08-23 13:14:42 PDT
Created attachment 654760 [details]
backtrace

Here's the backtrace, the first time IsCallerChrome returns false and triggers console-spam.
Comment 3 Daniel Holbert [:dholbert] 2012-08-23 13:21:38 PDT
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 [:bz] 2012-08-23 13:31:59 PDT
Hmm.  that makes sense for OnDiscard.  What's the stack for OnStartDecode?
Comment 5 Daniel Holbert [:dholbert] 2012-08-23 13:39:01 PDT
Created attachment 654772 [details]
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 [:bz] 2012-08-23 13:41:37 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-08-23 13:45:45 PDT
(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] 2012-08-23 13:48:28 PDT
(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] 2012-08-23 13:51:20 PDT
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] 2012-08-23 13:59:47 PDT
Created attachment 654782 [details] [diff] [review]
fix

This fixes the STR for me.
Comment 11 Boris Zbarsky [:bz] 2012-08-23 14:04:49 PDT
Comment on attachment 654782 [details] [diff] [review]
fix

r=me
Comment 12 Daniel Holbert [:dholbert] 2012-08-23 14:09:04 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fa18001d0a76
Comment 13 Olli Pettay [:smaug] 2012-08-23 14:12:05 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] 2012-08-23 14:15:09 PDT
(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] 2012-08-23 16:29:01 PDT
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] 2012-08-24 20:06:01 PDT
https://hg.mozilla.org/mozilla-central/rev/6e67b20609d2

Note You need to log in before you can comment on or make changes to this bug.