Closed
Bug 1230385
Opened 5 years ago
Closed 5 years ago
[e10s] Remove use of NS_NOTREACHED from TabChild.cpp and ContentChild.cpp
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(3 files)
1.05 KB,
patch
|
Details | Diff | Splinter Review | |
6.21 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
3.62 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
The HTML5 spec says that the |visible| property on BarProps is readonly, but this is not true of our implementation. Instead, it is only defacto readonly from non-chrome, due to a call to IsCallerChrome() call at the entry to the method. From reading code, I think this does not work with e10s, but does work without it. With e10s it will call into TabChild::SetChromeFlags(), which does not do anything. If we can do it without breaking anybody, I think it makes sense to make these properties truly readonly. I'm not sure how well setting these after window creation works in practice anyways. Note that scrollbars is its own can of worms, using a separate mechanism, which also does not work with e10s (bug 1229220). (It sets stuff on the current docshell, rather than on the chromeflags.)
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
The test in the attachment passes without e10s and fails with it.
Updated•5 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 3•5 years ago
|
||
It probably makes the most sense to not try to make this work. I can't see any code that does this in tree (and we'd get an assertion if it did). However, the NS_NOTREACHED should be replaced with something else, because it certainly can be reached, and technically I think the compiler is allowed to carry out undefined behavior if this code is run. Really, all of the NS_NOTREACHED in this file should be replaced with NS_WARNING or MOZ_ASSERT or something.
Assignee: nobody → continuation
Assignee | ||
Comment 4•5 years ago
|
||
I'm going to assume that the NS_NOTREACHED in ContentChild (which appear in various AllocPFooChild() methods) are similarly bogus.
No longer depends on: 1231498
Summary: [e10s] Setting non-scrollbars barprops in the child process as a chrome caller does not work → [e10s] Remove use of NS_NOTREACHED from TabChild.cpp and ContentChild.cpp
Assignee | ||
Comment 5•5 years ago
|
||
NS_NOTREACHED just has an annotation to the static analysis that it is never reached, not telling the compiler that like I thought, but I think this is still a little clearer. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=578f1f7eb576
Attachment #8705324 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 6•5 years ago
|
||
At least some of these methods can be called from chrome JS, so warn instead of having an inaccurate static analysis annotation about code never being reached.
Attachment #8705325 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 7•5 years ago
|
||
One thing to keep an eye out for in part 1 is that the final change for ContentChild::RecvUpdateWindow() isn't like the others. I just added a MOZ_ASSERT(false) because it isn't quite the same thing.
Comment on attachment 8705324 [details] [diff] [review] part 1 - Use MOZ_CRASH in ContentChild::AllocP*() methods. Review of attachment 8705324 [details] [diff] [review]: ----------------------------------------------------------------- I guess NS_NOTREACHED is another form of NS_ASSERTION. Kill it!
Attachment #8705324 -
Flags: review?(wmccloskey) → review+
Attachment #8705325 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Content Processes
Comment 9•5 years ago
|
||
this failed to apply: patching file dom/ipc/ContentChild.cpp Hunk #1 FAILED at 1565 Hunk #2 FAILED at 1592 Hunk #3 FAILED at 1697 Hunk #4 FAILED at 1753 Hunk #5 FAILED at 1786 Hunk #6 FAILED at 1825 Hunk #7 FAILED at 1844 Hunk #8 FAILED at 2000 Hunk #9 FAILED at 3009 9 out of 9 hunks FAILED -- saving rejects to file dom/ipc/ContentChild.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug-1230385-part-1---Use-MOZCRASH-in-ContentChildA.patch
Flags: needinfo?(continuation)
Keywords: checkin-needed
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(continuation)
Comment 10•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/45688153d6d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/212862056b47
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/45688153d6d1 https://hg.mozilla.org/mozilla-central/rev/212862056b47
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•