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

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Andrew McCreight [:mccr8]]

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.)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Test for setting barprops from chrome.

Comment 2

[Andrew McCreight [:mccr8]]

The test in the attachment passes without e10s and fails with it.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

I'm going to assume that the NS_NOTREACHED in ContentChild (which appear in various AllocPFooChild() methods) are similarly bogus.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Use MOZ_CRASH in ContentChild::AllocP*() methods.

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

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Use NS_WARNING in unimplemented TabChild methods.

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.

Comment 7

[Andrew McCreight [:mccr8]]

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 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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!

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45688153d6d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/212862056b47

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/45688153d6d1
https://hg.mozilla.org/mozilla-central/rev/212862056b47