Closed Bug 842561 Opened 8 years ago Closed 8 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + fixed
firefox21 + fixed
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main20-])

Attachments

(1 file, 1 obsolete file)

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...
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.
Attachment #715516 - Flags: review?(peterv)
Could this be a static assert?
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.
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.
Attached patch With that fixedSplinter Review
Attachment #715573 - Flags: review?(peterv)
Attachment #715516 - Attachment is obsolete: true
Attachment #715516 - Flags: review?(peterv)
Attachment #715573 - Flags: review?(peterv) → review+
Depends on: 842089
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
Attachment #715573 - Flags: approval-mozilla-beta?
Attachment #715573 - Flags: approval-mozilla-aurora?
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla22
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
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 on attachment 715573 [details] [diff] [review]
With that fixed

passes tests, low risk, early enough in the cycle to take this for uplift.
Attachment #715573 - Flags: approval-mozilla-beta?
Attachment #715573 - Flags: approval-mozilla-beta+
Attachment #715573 - Flags: approval-mozilla-aurora?
Attachment #715573 - Flags: approval-mozilla-aurora+
(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
Ryan, this bug depends on bug 842089; see comment 0 and dependencies.  Did you push it before pushing that bug?
(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
Whiteboard: [adv-main20-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.