842561 - Assert/ensure that the inheritance chain for DOM objects matches the interface inheritance chain

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This bit us in bug 842089.

I'm adding some checks which make the code in that bug not compile.  They _also_ make CDATASection not compile, hence the security-sensitive bit.

I _think_ in practice the code in CDATASection is maybe safe.  Maybe.  I still think we should take this on 20 and 21, which is where bug 826703 was fixed...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Assert that our inheritance chain is correct for WebIDL objects. could drop the descriptor for Text if we changed nsIDocument::CreateTextNode

to return an already_AddRefed<Text>, but then we'd need more casting in
nsDocument.cpp for the XPCOM CreateTextNode.  Not sure which way is better,
really.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=724f14bb5a81

Comment 3

[Jesse Ruderman]

Could this be a static assert?

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, because you can't use static_cast in static asserts.  Yes, that's moronic; that's C++ for you.

The assert code here will fail to compile at all in cases like bug 842089, so will catch those at compile time.  It'll only be able to catch incorrect object layout at runtime, though.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That fails the asserts because nsContentList does not in fact reinterpret_cast to nsIHTMLCollection.  But I guess that requirement is too strict in any case.  Fix coming up that doesn't impose it.

Comment 6

[Andrew McCreight [:mccr8]]

Pessimistically marking this sec-high, based on comment 0.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: With that fixed

Comment 8

[Boris Zbarsky [:bzbarsky]]

New try run: https://tbpl.mozilla.org/?tree=Try&rev=362dd168fb25

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715573 [details] [diff] [review]
With that fixed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 826703
User impact if declined: Risk of exploitable crashes, though I'm not entirely
   certain this can actually happen.
Testing completed (on m-c, etc.): Passes our regression tests.
Risk to taking this patch (and alternatives if risky): I believe the risk here is
   fairly low.
String or UUID changes made by this patch: None

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b189c8951b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was backed out due to one of the changesets in the push making browser_dbg_bug723069_editor-breakpoints.js fail frequently on Windows and OSX opt builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca34c11bf55d

https://tbpl.mozilla.org/php/getParsedLog.php?id=19995636&tree=Mozilla-Inbound

11:12:16     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | editor.getBreakpoints().length is correct
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | remove the second breakpoint using the mouse
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of attributes' specified attribute is deprecated. It always returns true." {file: "chrome://browser/content/orion.js" line: 6341}]
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of removeAttributeNode() is deprecated. Use removeAttribute() instead." {file: "chrome://browser/content/orion.js" line: 6342}]
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:46  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Test timed out
11:12:46  WARNING -  This is a harness error.
11:12:46     INFO -  args: ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/mozilla-test-fail_GzeI8j']
11:12:48     INFO -  SCREENSHOT: <see log>
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ToolbarView
11:12:48     INFO -  DBG-FRONTEND: Destroying the OptionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ChromeGlobalsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the SourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilterView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilteredSourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the StackFramesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the BreakpointsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the WatchExpressionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the GlobalSearchView
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView window
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView panes
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView editor
11:12:48     INFO -  DBG-FRONTEND: SourceScripts is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: StackFrames is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: ThreadState is disconnecting...
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been added
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been removed
11:12:48  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of editor breakpoint changes - Got 3, expected 4
11:12:48  WARNING -  This is a harness error.
11:12:48     INFO -  Stack trace:
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 486
11:12:48     INFO -      JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js :: <TOP_LEVEL> :: line 295
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_nextTest :: line 250
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 419
11:12:48     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
11:12:48     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | finished in 30099ms

Comment 12

[Boris Zbarsky [:bzbarsky]]

Confirmed via try that this bug is not responsible for that orange (as far as I can tell) and pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/62f3d4a4421a

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

 https://hg.mozilla.org/mozilla-central/rev/62f3d4a4421a

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 715573 [details] [diff] [review]
With that fixed

passes tests, low risk, early enough in the cycle to take this for uplift.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e485a12e27ae
https://hg.mozilla.org/releases/mozilla-beta/rev/72293b033da5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e485a12e27ae
> https://hg.mozilla.org/releases/mozilla-beta/rev/72293b033da5

Backed out for debug bustage. The beta backout might have been a bit premature, but I decided better to keep the two together.
https://hg.mozilla.org/releases/mozilla-aurora/rev/31d3b7ffa704
https://hg.mozilla.org/releases/mozilla-beta/rev/771f5f0cfce6

https://tbpl.mozilla.org/php/getParsedLog.php?id=20069736&tree=Mozilla-Aurora

MutationObserverBinding.cpp
LocalMediaStreamBinding.cpp: In function 'JSObject* mozilla::dom::LocalMediaStreamBinding::Wrap(JSContext*, JSObject*, mozilla::DOMLocalMediaStream*, nsWrapperCache*, bool*)':
LocalMediaStreamBinding.cpp:242:61: error: invalid static_cast from type 'mozilla::DOMLocalMediaStream*' to type 'mozilla::dom::EventTarget*'
make[6]: *** [LocalMediaStreamBinding.o] Error 1
make[6]: *** Waiting for unfinished jobs....
MediaStreamBinding.cpp: In function 'JSObject* mozilla::dom::MediaStreamBinding::Wrap(JSContext*, JSObject*, mozilla::DOMMediaStream*, nsWrapperCache*, bool*)':
MediaStreamBinding.cpp:241:61: error: invalid static_cast from type 'mozilla::DOMMediaStream*' to type 'mozilla::dom::EventTarget*'
make[6]: *** [MediaStreamBinding.o] Error 1
make[6]: Leaving directory `/builds/slave/m-aurora-lx-d-0000000000000000/build/obj-firefox/dom/bindings'
make[5]: *** [bindings_libs] Error 2

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ryan, this bug depends on bug 842089; see comment 0 and dependencies.  Did you push it before pushing that bug?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Ryan, this bug depends on bug 842089; see comment 0 and dependencies.  Did
> you push it before pushing that bug?

Argh, fail.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b56e7dcd0c6
https://hg.mozilla.org/releases/mozilla-beta/rev/134b35ff1447