155246 - [PATCH] ASSERTION: failed to load URL; FrameLoader Asserts load success though nsIContentPolicy could reject it

Status: NEW

(Core :: Layout: Images, Video, and HTML Frames, defect, P3)

Description

[Tim]

CVS trunk build from 2002-6-29 (Athlon/Debian/Sid, but not relevant)

I was working in extensions/cookie to test the ability to apply nsIContentPolicy
blocking to (sub)documents, and I hit the following assertion.  The first line
is something I added for debugging, while the rest are from CVS (I manually
added spaces to avoid widening the page).

(SUBDOCUMENT: mediamgr.ugo.com/html.ng/size=468x60&affiliate=bacon
&channel=freestyle&subchannel=freestyle&Network=affiliates&rating=pg13) BLOCKED
###!!! ASSERTION: failed to load URL: 'NS_SUCCEEDED(rv)', file
nsFrameLoader.cpp, line 230
Break: at file nsFrameLoader.cpp, line 230

Convenience link:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsFrameLoader.cpp#230

The frame loader should probably not assert successful loading since LoadURI
consults the nsIContentPolicy chain and thus can legitimately fail.

This may be tangentially related to bug 147741, since it would affect embedders.


If you want to see it for yourself, you need to rig
extensions/cookie/nsImgManager.cpp to accept nsIContentPolicy::SUBDOCUMENT in
the switch statement (line 115) wherein it sets *_retval to PR_FALSE and returns
successfully.  Then load a document which contains subdocuments
(www.ilovebacon.com is one of many such sites).

If anyone expresses interest (which I doubt), I can turn what I just said into a
testcase patch.  However, I hope my description has been detailed enough to
stand on its own.

Comment 1

[Tim]

Attachment: Remove the assertion

Since I've never received this assertion except when my subdocument-blocking
nsIContentPolicy-based filter takes effect, it may not be necessary any more. 
Alternatively, I could attach a patch which turns it into a NS_WARNING.

Comment 2

[Tim]

Attachment: Alternate: NS_WARN_IF_FALSE

Alternate version that, instead of removing all notice, turns the assertion
into a (less annoying) warning.

Comment 3

[John Keiser (jkeiser)]

What does LoadURI return when nsIContentPolicy blocks the subdocument?  We
should keep the assertion but not assert on legitimate failure.  Maybe
NS_SECURITY_ERROR or something?

Comment 4

[Tim]

You'll love this: it returns NS_ERROR_FAILURE (0x80004005). *sigh*

Comment 5

[Tim]

I've added bug 157215 to cover docshell giving a better exception for this
situation.

Comment 6

[Kevin McCluskey (gone)]

Bulk moving P1-P5 un-milestoned bugs to future. 

Comment 7

[Son Le]

Attachment: Alternative 2: add content block error codes to assertion check

Add NS_ERROR_CONTENT_BLOCKED and NS_ERROR_CONTENT_BLOCKED_SHOW_ALT to assertion
check. This works for me.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

(From update of attachment #165887 [details] [diff] [review])
Hm.  I suppose what confuses me is if this is not really a failure case, why are
we returning it as a failure.  We have perfectly valid success codes we can be
returning instead.  See NS_CONTENT_BLOCKED and NS_CONTENT_BLOCKED_SHOW_ALT.  It
seems to me that one of nsIContentPolicy's purpose is to intentionally block
some content, so when it fulfills its purpose, it should return a success code,
not a failure....  could someone shed some light on this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

The assert is correct, somewhat.

At the very least, errors due to content policy should not be propagated out of
this method (and I see a similar problem for the security manager check).  An
error result out of here will (for <xul:iframe> and <xul:browser>) propagate out
to frame construction, and cut off rendering of the document entirely, if the
frame constructor is not sucking that day.  That's hardly desirable...

caillon, to answer your question, sometimes an error return is used because code
doesn't properly bail out on non-error returns and changing error checking all
over to bail out once a load has been denied is a lot of work.  So the error
codes should be converted to success at selected chokepoints.

Comment 10

[Jesse Ruderman]

Bug 322774 has another way to trigger the same assertion message.