Last Comment Bug 393503 - [FIX]Disabling images doesn't work for <object> with no type attribute
: [FIX]Disabling images doesn't work for <object> with no type attribute
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
data:text/html,<object src="http://ww...
Depends on: 393410
Blocks: 1156
  Show dependency treegraph
Reported: 2007-08-23 21:55 PDT by Boris Zbarsky [:bz]
Modified: 2012-08-10 17:09 PDT (History)
5 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed fix (7.19 KB, patch)
2007-08-23 22:20 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
jst: approval1.9+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2007-08-23 21:55:54 PDT
BUILD: Current trunk


1) Disable image loading in preferences
2) Load the URL of this bug

ACTUAL RESULTS: Image shows up

Comment 1 Boris Zbarsky [:bz] 2007-08-23 22:20:57 PDT
Created attachment 278005 [details] [diff] [review]
Proposed fix
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-24 09:30:10 PDT
Comment on attachment 278005 [details] [diff] [review]
Proposed fix

+  if (NS_FAILED(rv) || NS_CP_REJECTED(shouldProcess)) {
+    HandleBeingBlockedByContentPolicy(rv, shouldProcess);

I think you want to set rv to NS_OK here to avoid the AutoFallback calling Fallback (LoadObject doesn't have the issue because the AutoFallback gets instantiated after the contentpolicy check)
Comment 3 Boris Zbarsky [:bz] 2007-08-24 15:25:13 PDT
Comment on attachment 278005 [details] [diff] [review]
Proposed fix

Good catch.  Will do that on checkin.

Approvers: this is fairly low-risk, and allows image blocking to work reliably for <object> at least in terms of not showing the image (and for large enough images also not doing most of the load).
Comment 4 Boris Zbarsky [:bz] 2007-09-14 11:58:53 PDT
Checked in with the rv change there.  Need some tests, somehow...  Ideally something where we set prefs (a la mochitest) then snapshot (a la reftest).
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2007-09-14 14:59:46 PDT
Ideally reftest would have a custom profile environment where we could set prefs to allow use of enablePrivilege and such; I suppose that's bug 393410.  With that capability (pun intended!), this would be easy enough to do in a reftest.
Comment 6 Boris Zbarsky [:bz] 2007-09-14 15:03:53 PDT
Yeah, that would do it.  In general, if we could run reftest like we do mochitest (against an http server, on a new profile) that would be great.

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