bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(e10s+, firefox45 affected, firefox46 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8696063 [details] [diff] [review]
Test for setting barprops from chrome.
(Assignee)

Comment 2

3 years ago
The test in the attachment passes without e10s and fails with it.
(Assignee)

Updated

3 years ago
Depends on: 1231498
tracking-e10s: --- → +
(Assignee)

Comment 3

3 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

3 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

3 years ago
Created attachment 8705324 [details] [diff] [review]
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
Attachment #8705324 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

3 years ago
Created attachment 8705325 [details] [diff] [review]
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.
Attachment #8705325 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

3 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

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
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
(Assignee)

Updated

3 years ago
Flags: needinfo?(continuation)

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45688153d6d1
https://hg.mozilla.org/mozilla-central/rev/212862056b47
Status: NEW → RESOLVED
Last Resolved: 3 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.