Closed Bug 622259 Opened 13 years ago Closed 13 years ago

In Yahoo Mail! Unable to activate Show Images in Spam Folder

Categories

(Core :: XPConnect, defect)

x86
All
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: sunveer, Assigned: smaug)

References

()

Details

(Keywords: regression, Whiteboard: [hardblocker] [fx4-fixed-bugday])

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

In the screenshot that I have provided in spam there's always a option to show images by clicing on Show Images button in yellow bar but there is no effect on clicking on this button.

Reproducible: Always

Steps to Reproduce:
1.Open Yahoo mail Spam folder
2. Click on Show Images
3.
Actual Results:  
No effect on clicking.
Build worked : 
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre
http://hg.mozilla.org/mozilla-central/rev/2593c8c8af8b

Build broken:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101013 Firefox/4.0b8pre
http://hg.mozilla.org/mozilla-central/rev/29c228a4d7eb

Pushlog : 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2593c8c8af8b
&tochange=29c228a4d7eb
Status: UNCONFIRMED → NEW
Ever confirmed: true
In local build
1eda0cc3bbf9 : fails
659215248912 : works
Blocks: 580128
Keywords: regression
Component: General → XPConnect
Product: Firefox → Core
QA Contact: general → xpconnect
Target Milestone: --- → mozilla2.0
Version: unspecified → Trunk
Blake, can you take a look?
blocking2.0: --- → ?
Target Milestone: mozilla2.0 → ---
What happened about the problem??
Mounir, could you help trace down what's failing here? From the regression range this broke during the massive compartment landing.
Assignee: nobody → mounir.lamouri
blocking2.0: ? → final+
Whiteboard: [softblocker]
Last good nightly: 2010-10-13 First bad nightly: 2010-10-14

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=178f26e21cfc&tochange=ad0a0be8be74

With no surprise, it's a Compartment JS regression.
Still in Firefox 4 Beta 9 Clicking on Images in Spam folder doesn't have any action.
Hmm, I didn't see last time that there is a "Not same origin!" warning just before the javascript error. I'm going to check what trigger this warning (assuming the error comes from the warning).
OS: Windows XP → All
Still in Firefox 4 Beta 4, only for me it is with every mail from yahoo mail, not only with those in the spam folder.
(In reply to comment #13)
> Still in Firefox 4 Beta 4, only for me it is with every mail from yahoo mail,
> not only with those in the spam folder.

Firefox 4 Beta 10,  sorry!
Still in 4 beta 10 bug is there.
I'm trying to isolate the issue but it's a real pain given that the js code is minified. The only think I've been able to see is with the stack: the "persmission denied" error comes from the click handler AFAICT.

Is there anything I could try to make the investigations easier?
Andreas says he'll look into this. Reassigning.
Assignee: mounir.lamouri → gal
Do I need the classic yahoo mail or the new one? And I can't find a show images button. Some more detailed STRs would be great.
Actually never mind I got it:

#0  xpc::AccessCheck::deny (cx=0x7fffdc697800, id=...) at ../../../../../js/src/xpconnect/wrappers/AccessCheck.cpp:411
#1  0x00007ffff5b0b042 in CheckAndReport<xpc::OnlyIfSubjectIsSystem> (cx=0x7fffdc697800, wrapper=0x7fffcc7085b0, id=..., act=JSWrapper::GET, 
    perm=@0x7fffffff9cac) at ../../../../../js/src/xpconnect/wrappers/FilteringWrapper.cpp:101
#2  0x00007ffff5b0d6b4 in xpc::FilteringWrapper<JSWrapper, xpc::OnlyIfSubjectIsSystem>::enter (this=0x7ffff7b954a0, cx=0x7fffdc697800, 
    wrapper=0x7fffcc7085b0, id=..., act=JSWrapper::GET) at ../../../../../js/src/xpconnect/wrappers/FilteringWrapper.cpp:149
#3  0x00007ffff6677ac2 in JSWrapper::obj_toString (this=0x7ffff7b954a0, cx=0x7fffdc697800, wrapper=0x7fffcc7085b0) at ../../../js/src/jswrapper.cpp:263
#4  0x00007ffff6619b63 in js::JSProxy::obj_toString (cx=0x7fffdc697800, proxy=0x7fffcc7085b0) at ../../../js/src/jsproxy.cpp:852
#5  0x00007ffff65c3671 in js::obj_toStringHelper (cx=0x7fffdc697800, obj=0x7fffcc7085b0) at ../../../js/src/jsobj.cpp:822
#6  0x00007ffff65c3898 in obj_toString (cx=0x7fffdc697800, argc=0, vp=0x7fffe2efe360) at ../../../js/src/jsobj.cpp:870
#7  0x00007ffff65ad6d0 in js::CallJSNative (cx=0x7fffdc697800, native=0x7ffff65c37a4 <obj_toString>, argc=0, vp=0x7fffe2efe360)
    at ../../../js/src/jscntxtinlines.h:697
#8  0x00007ffff65a98e4 in js::Invoke (cx=0x7fffdc697800, argsRef=..., flags=0) at ../../../js/src/jsinterp.cpp:700
#9  0x00007ffff6583a13 in js_fun_call (cx=0x7fffdc697800, argc=0, vp=0x7fffe2efe348) at ../../../js/src/jsfun.cpp:2115
Andreas Gal the beta version of Yahoo mail does not have the problem. Its the
regular version of Yahoo mail that replaced Yahoo Classic mail. If you get an
email with graphics Yahoo hides the graphics if under the Spam setting for
Yahoo mail the "Initially block all images" is enabled.
So what happens here is that GetRangeParent returns native anonymous content and we wrap that into a system-only wrapper and then throw. Either GetRangeParent should not return this node, or the event code should filter this result. Over to smaug to figure out what to do with this.

(gdb) bt 10
#0  GetSelectionClosestFrameForLine (aParent=0x7fffcf1eaa58, aLine=..., aPoint=...) at ../../../layout/generic/nsFrame.cpp:2752
#1  0x00007ffff4e42d3b in GetSelectionClosestFrameForBlock (aFrame=0x7fffcf1eaa58, aPoint=...) at ../../../layout/generic/nsFrame.cpp:2862
#2  0x00007ffff4e42e9e in GetSelectionClosestFrame (aFrame=0x7fffcf1eaa58, aPoint=...) at ../../../layout/generic/nsFrame.cpp:2882
#3  0x00007ffff4e420cc in GetSelectionClosestFrameForChild (aChild=0x7fffcf1eaa58, aPoint=...) at ../../../layout/generic/nsFrame.cpp:2704
#4  0x00007ffff4e43143 in GetSelectionClosestFrame (aFrame=0x7fffe1118020, aPoint=...) at ../../../layout/generic/nsFrame.cpp:2937
#5  0x00007ffff4e435ef in nsIFrame::GetContentOffsetsFromPoint (this=0x7fffe1118020, aPoint=..., aIgnoreSelectionStyle=0)
    at ../../../layout/generic/nsFrame.cpp:3028
#6  0x00007ffff5216b55 in nsDOMUIEvent::GetRangeParent (this=0x7fffd09440c0, aRangeParent=0x7fffffff9af0)
    at ../../../../content/events/src/nsDOMUIEvent.cpp:271
#7  0x00007ffff5a90162 in nsIDOMNSUIEvent_GetRangeParent (cx=0x7fffdb247c00, obj=0x7fffd0520688, id=..., vp=0x7fffffffa060) at dom_quickstubs.cpp:15641
#8  0x00007ffff65d7900 in js::CallJSPropertyOp (cx=0x7fffdb247c00, op=0x7ffff5a90019 <nsIDOMNSUIEvent_GetRangeParent>, obj=0x7fffd0520688, id=..., 
    vp=0x7fffffffa060) at ../../../js/src/jscntxtinlines.h:741
#9  0x00007ffff65d80d6 in js::Shape::get (this=0x7fffd41950b8, cx=0x7fffdb247c00, receiver=0x7fffd0520688, obj=0x7fffd0520688, pobj=0x7fffd0c2ce70, 
    vp=0x7fffffffa060) at ../../../js/src/jsscopeinlines.h:256
Assignee: gal → Olli.Pettay
Whiteboard: [softblocker] → [hardblocker]
Um, I wonder why compartments caused this problem.
We have thrown in this case for a long time. Why does it cause a problem
now with compartments?
Andreas thought we leaked native anonymous content. On 3.6 do we definitely throw and not leak?

/be
By "leak" I mean allow access from the page to NAC.

/be
On 3.6 we don't give access to NAC. 
https://bugzilla.mozilla.org/show_bug.cgi?id=475864
"SystemOnlyWrapper"
...but, returning null from nsDOMUIEvent::GetRangeParent does seem to fix
Yahoo mail.

Did compartments somehow change the behavior of SystemOnlyWrapper?
FF3.6 throws
Exception... "Security Manager vetoed action"  nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)"

Trunk
Error: Permission denied to access property 'valueOf'
We should never leak (meaning allow to escape; and wrap and error, but leak is the first bug) NAC, though. Right?

/be
I like my error message better. I can try the old one and see if yahoo eats that, but anyway, the bug fix here is to not hand out NAC, so lets go with that.
Returning null has some regression risk, since then we need to check if
some content JS is running, but what if the code calling GetRangeParent is C++, 
which actually should be able to access NAC.
FYI, making GetRangeParent to throw NS_ERROR_FAILURE doesn't seem to fix Yahoo mail.
I'll post the "return null" patch to tryserver to see if it breaks something there.
Blake observed that the code is doing Object.prototype.toString.call(NAC) and that used to work.

It's an old way to tell the "type" of a JS object, for almost any object:

js> NAC=/hi/
/hi/
js> Object.prototype.toString.call(NAC)
"[object RegExp]"
js> Object.prototype.toString.call(NAC).slice(8,-1)
"RegExp"

This shows how the web finds all your incompatibilities.

/be
http://hg.mozilla.org/try/rev/993d0734e609
seems to pass the tests on tryserver.
The problem is that I don't exactly know why the patch helps in this case.
The js code is very obscure and my js/xpconnect-debugging-fu could be better.
But I can continue tomorrow, unless someone more familiar with jseng/xpconnect
could check the js code.

Also, it is not clear to me whether we should fix SystemOnlyWrapper
handling regression, or just GetRangeParent.
Is comment 34 accurate? If so, giving a safe toString to SOWs might be worth the trouble. This is one mole we've whacked. How many more? No idea but it's likely too late to find out for some higher value of "for sure", and restore compat for Fx4.

If comment 34 in its particulars is not accurate, please say more.

/be
I was hoping to see the actual code.
But ok, Object.prototype.toString.call(null) does work.


I'll post a patch.
Attached patch return nullSplinter Review
I need to write still a testcase for this.
The simplest mochitest approach didn't work since that gave too much
privileges to event listener, so I guess I need rewrite that as a mochitest
which doesn't need to explicitly call netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect")
to be able to dispatch events via DOMWindowUtils.
Attachment #507873 - Flags: review?(jst)
Comment on attachment 507873 [details] [diff] [review]
return null

     if (parent) {
+      if (parent->IsInNativeAnonymousSubtree() &&
+          !nsContentUtils::CanAccessNativeAnon()) {
+        return NS_OK;
+      }
       return CallQueryInterface(parent, aRangeParent);
     }
   }
 
   return NS_OK;

Maybe write that as:

    if (parent && (!parent->IsInNativeAnonymousSubtree ||
                   nsContentUtils::CanAccessNativeAnon()))
      return CallQueryInterface(parent, aRangeParent);
    }

    return NS_OK;

? Either way, r=jst
Attachment #507873 - Flags: review?(jst) → review+
... with correct bracing, that is.
Drive-by, jst can take it :-P.

The right-heavy

  if (pooh && (tigger || rabbit || piglet || owl || kanga || roo ||
               eeyore)) {
  }

seems ugly to me. Plus, even though CQI is null-safe I like the way Smaug did it.

/be
I wasn't going for pretty as much as eliminating an avoidable early return. But as I said, I'm fine either way. :)
CQI is not null-safe, fwiw.
(In reply to comment #43)
> CQI is not null-safe, fwiw.

My XPCOM helper memory is shot. Good!

/be
http://hg.mozilla.org/mozilla-central/rev/fc363326e0ef
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker] [fx4-fixed-bugday]
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre

Confirming issue is fixed.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: