Closed Bug 1230385 Opened 7 years ago Closed 6 years ago

[e10s] Remove use of NS_NOTREACHED from TabChild.cpp and ContentChild.cpp


(Core :: DOM: Content Processes, defect)

Not set



Tracking Status
e10s + ---
firefox45 --- affected
firefox46 --- fixed


(Reporter: mccr8, Assigned: mccr8)



(3 files)

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.)
The test in the attachment passes without e10s and fails with it.
Depends on: 1231498
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
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
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:
Attachment #8705324 - Flags: review?(wmccloskey)
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)
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+
Keywords: checkin-needed
Component: DOM → DOM: Content Processes
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
Flags: needinfo?(continuation)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.