Closed Bug 1002313 Opened 10 years ago Closed 10 years ago

Fix the test for bug 503926 so that it doesn't rely on nsIDOMNode being scriptable

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

Attachment #8413510 - Flags: review?(bzbarsky)
Comment on attachment 8413510 [details] [diff] [review]
Fix the test for bug 503926 so that it doesn't rely on nsIDOMNode being scriptable; r=bzbarsky

r=me if you add tests to all the catch clauses that the thrown exception is about the object not being an nsIDOMWindowUtils object or whatever the exception message is (e.g. to make sure that in test_bug503926.html we're not getting an exception from one of the bits before the last '.' in the try.
Attachment #8413510 - Flags: review?(bzbarsky) → review+
So attempting to verify the exceptions in the test, I realized that none of these tests doesn't even throw in this case, and XpconnectArgument() ends up being called with an nsXPTCStubBase* like this:

   3993	nsDOMWindowUtils::XpconnectArgument(nsIDOMWindowUtils* aThis)
   3994	{
   3995	  // Do nothing.
-> 3996	  return NS_OK;
   3997	}
   3998
   3999	NS_INTERFACE_MAP_BEGIN(nsTranslationNodeList)
(lldb) p aThis
(nsXPTCStubBase *) $0 = 0x00000001300f0d80
(lldb) bt 10
* thread #1: tid = 0xcc45f, 0x00000001034c0671 XUL`nsDOMWindowUtils::XpconnectArgument(this=0x000000011e5981c0, aThis=0x00000001300f0d80) + 17 at nsDOMWindowUtils.cpp:3996, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00000001034c0671 XUL`nsDOMWindowUtils::XpconnectArgument(this=0x000000011e5981c0, aThis=0x00000001300f0d80) + 17 at nsDOMWindowUtils.cpp:3996
    frame #1: 0x00000001016c3a89 XUL`NS_InvokeByIndex(that=0x000000011e5981c0, methodIndex=133, paramCount=1, params=0x00007fff5fbf2fe8) + 537 at xptcinvoke_x86_64_unix.cpp:162
    frame #2: 0x00000001033d30a4 XUL`CallMethodHelper::Invoke(this=0x00007fff5fbf2fa0) + 84 at XPCWrappedNative.cpp:2392
    frame #3: 0x00000001033c71e7 XUL`CallMethodHelper::Call(this=0x00007fff5fbf2fa0) + 263 at XPCWrappedNative.cpp:1733
    frame #4: 0x00000001033a6092 XUL`XPCWrappedNative::CallMethod(ccx=0x00007fff5fbf3188, mode=CALL_METHOD) + 226 at XPCWrappedNative.cpp:1700
    frame #5: 0x00000001033a86c6 XUL`XPC_WN_CallMethod(cx=0x0000000127d78480, argc=1, vp=0x00007fff5fbf3e80) + 918 at XPCWrappedNativeJSOps.cpp:1285
    frame #6: 0x00000001064a1865 XUL`js::CallJSNative(cx=0x0000000127d78480, native=0x00000001033a8330, args=0x00007fff5fbf38c0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:239
    frame #7: 0x0000000106476fba XUL`js::Invoke(cx=0x0000000127d78480, args=CallArgs at 0x00007fff5fbf38c0, construct=NO_CONSTRUCT) + 1178 at Interpreter.cpp:475
    frame #8: 0x00000001062889a3 XUL`js_fun_apply(cx=0x0000000127d78480, argc=2, vp=0x000000011256e210) + 1603 at jsfun.cpp:1020
    frame #9: 0x00000001064a1865 XUL`js::CallJSNative(cx=0x0000000127d78480, native=0x0000000106288360, args=0x00007fff5fbf44f0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:239

What should I do given that?
Flags: needinfo?(bzbarsky)
I think it's time to ask mrbkap what he expects to be happening here...
Flags: needinfo?(bzbarsky) → needinfo?(mrbkap)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> So attempting to verify the exceptions in the test, I realized that none of
> these tests doesn't even throw in this case, and XpconnectArgument() ends up
> being called with an nsXPTCStubBase* like this:

Now that you have a function that takes a (pretty much) arbitrary object, there won't be any more exceptions thrown. All that matters is whether we call QI on untrusted objects. That being said, the importance of the fix for bug 503926 is quickly diminishing with webidl. So I think you can get rid of the try/catches entirely.
Flags: needinfo?(mrbkap)
(In reply to comment #5)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #3)
> > So attempting to verify the exceptions in the test, I realized that none of
> > these tests doesn't even throw in this case, and XpconnectArgument() ends up
> > being called with an nsXPTCStubBase* like this:
> 
> Now that you have a function that takes a (pretty much) arbitrary object, there
> won't be any more exceptions thrown. All that matters is whether we call QI on
> untrusted objects. That being said, the importance of the fix for bug 503926 is
> quickly diminishing with webidl. So I think you can get rid of the try/catches
> entirely.

Sounds good, thanks!
https://hg.mozilla.org/mozilla-central/rev/e18d4a0e3bbc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.