Closed Bug 1152479 Opened 5 years ago Closed 5 years ago

Non-chrome-privileged code calls into APZCCallbackHelper::UpdateRootFrame()

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(6 files, 2 obsolete files)

Backtrace
32.42 KB, text/plain
Details
A successful backtrace
4.98 KB, text/plain
Details
MozReview Request: Bug 1152479 - Remove nsIDOMWindowUtils::isFirstPaint, as it only had C++ users. r=ehsan
39 bytes, text/x-review-board-request
ehsan
: review+
botond
: review+
Details
MozReview Request: Bug 1152479 - Extract the implementations of nsIDOMWindowUtils::Send{KeyEvent,MouseEvent} into nsContentUtils. r=ehsan
39 bytes, text/x-review-board-request
ehsan
: review+
botond
: review+
Details
MozReview Request: Bug 1152479 - In C++ APZ code, use nsContentUtils rather than nsIDOMWindowUtils to send key and mouse events. r=kats
39 bytes, text/x-review-board-request
ehsan
: review+
botond
: review+
Details
MozReview Request: Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan
39 bytes, text/x-review-board-request
ehsan
: review+
botond
: review+
Details 
The patch in bug 1039818 adds a call to nsIDOMWindowUtils::GetResolution() in APZCCallbackHelper::UpdateRootFrame(). The implementation of GetResolution() asserts that the caller has chrome privileged [1].

In the Try push for that patch, the assertion is failing for some tests [2].

This is worrisome, because even without that patch, UpdateRootFrame() calls nsIDOMWindowUtils::SetResolutionAndScaleTo() [3], which also checks from chrome privileges, but returns an error instead of assertion if the caller doesn't have them [4], a failure which UpdateRootFrame() silently ignores.

So, this failure has actually alerted us to a pre-existing issue.

I find it unexpected that code without chrome privileges gets into UpdateRootFrame() to begin with.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=c0937432d949#537
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=74dc3dc919e4
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=600015044b60#212
[4] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=c0937432d949#517
Blocks: 1039818
I haven't been able to repro this in a desktop mochitest or in the b2g browser, so I'm going to spin up an emulator build to try and repro it in the b2g mochitest.
Attached file Backtrace (obsolete) — 
Here's the backtrace that shows how it's happening, taken from the above try run.
Attached file Backtrace — 
Copy/paste fail
Attachment #8589866 - Attachment is obsolete: true
Thanks - the backtrace is basically all I wanted out of a repro, I didn't realize the try log already had it!

I talked this over with Ehsan, and it looks like we have a pretty horrifying state of affairs on our hands:

  - IsCallerChrome() returns true only if there is chrome-privileged JS
    on the stack. It returns false if the JS on the stack is not chrome-
    privileged OR if there is no JS on the stack.

  - nsIDOMWindowUtils isn't meant to be used from C++ code (nor from
    non-privileged JS, obviously).

  - To this end, nsIDOMWindowUtils functions all either check or assert
    IsCallerChrome().

  - There is nevertheless a lot of code out there, including in APZ
    (with numerous new call sites having been added in the past few months!),
    which calls nsIDOMWindowUtils from C++ code.

  - These calls have been working by chance in cases where there happened
    to be chrome JS on the stack, and silently failing otherwise!

      - That there haven't been assertion failures isn't a 
        coincidence. Ehsan changed the checks to assertion failures
        only for functions for which doing so didn't trigger
        assertion failures.
(Ehsan: no reponse required, it'd just be a shame if you missed comment 5 :))
Flags: needinfo?(ehsan)
Attached file A successful backtrace — 
So here's the usual backtrace for calling into APZCCallbackHelper::UpdateRootFrame. In this case it went into nsDOMWindowUtils::SetScrollPositionClampingScrollPortSize and got past the IsCallerChrome check, although it's not obvious to me what code on the stack is chrome script.

Also it seems there's quite a lot of event loops in this stack.
Had some more discussions with Ehsan and others on this topic. Summary:

  - We were mistaken about IsCallerChrome() returning false if there
    is no JS code on the stack. It returns true in that case.

  - In the stack trace in comment 3, there is content JS on the stack
    (which calls into layout, which triggers a before-first-paint
    event, which is observed by TabChild); this is why the
    IsCallerChrome() check is returning false.

      - Given that the caller is C++ code, it's nevertheless safe
        to perform the operations we're trying to perform here.

  - nsIDOMWindowUtils is still not meant to be used from C++ code, and
    as needs to keep the IsCallerChrome() check for other reasons. C++
    code should perform the operation guarded by IsCallerChrome()
    directly.
No longer blocks: 1039818
Have some in-progress patches that stop using nsIDOMWindowUtils from APZ-related C++ code.
Assignee: nobody → botond
Attached file MozReview Request: bz://1152479/botond (obsolete) — 
/r/7043 - Bug 1152479 - Extract the implementations of layout-related nsIDOMWindowUtils APIs used by APZ into nsLayoutUtils. r=ehsan
/r/7045 - Bug 1152479 - Do not use layout-related nsIDOMWindowUtils APIs from C++ APZ code. r=kats
/r/7047 - Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan
/r/7049 - Bug 1152479 - Remove nsIDOMWindowUtils::isFirstPaint, as it only had C++ users. r=ehsan

Pull down these commits:

hg pull -r 8215c7b4e70792939dae0c318c816cdaa287238c https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592353 - Flags: review?(ehsan)
Attachment #8592353 - Flags: review?(bugmail.mozilla)
These patches transition APZ's use layout-related nsIDOMWindowUtils APIs (which is most of the nsIDOMWindowUtils APIs that APZ uses) from C++ code to use nsLayoutUtils (or nsIPresShell functions) directly instead.

There remain a couple of nsIDOMWindowUtils APIs in C++ APZ code (sendKeyEvent and sendMouseEvent), which I'll transition in a separate patch, probably to nsContentUtils.

Try push that includes these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e7237a0715f
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

/r/7043 - Bug 1152479 - Extract the implementations of layout-related nsIDOMWindowUtils APIs used by APZ into nsLayoutUtils. r=ehsan
/r/7045 - Bug 1152479 - Do not use layout-related nsIDOMWindowUtils APIs from C++ APZ code. r=kats
/r/7047 - Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan

Pull down these commits:

hg pull -r 714a9bb99d7696e5f00252adb1ddc11455c22594 https://reviewboard-hg.mozilla.org/gecko/
I was mistaken about isFirstPaint not having JS callers (I was searching for getIsFirstPaint/setIsFirstPaint rather than just isFirstPaint), so I dropped the patch when removed it from nsIDOMWindowUtils.
https://reviewboard.mozilla.org/r/7045/#review5857

::: dom/ipc/TabChild.cpp
(Diff revision 1)
> -    utils->SetCSSViewport(aSize.width, aSize.height);
> +    if (nsIPresShell* shell = doc->GetShell()) {

I think doc could be null here. It might be better to extract a helper GetPresShell() function which guards against that in one place.

::: dom/ipc/TabChild.cpp
(Diff revision 1)
> +  if (!shell || isFirstPaint) {

This is a bit confusing. Would prefer:

bool isFirstPaint = true;
if (shell) {
  isFirstPaint = shell->GetIsFirstPaint();
}
if (isFirstPaint) {
  ...

::: dom/ipc/TabChild.cpp
(Diff revision 1)
> -    if (APZCCallbackHelper::HasValidPresShellId(utils, aFrameMetrics)) {
> +      if (nsIPresShell* shell = doc->GetShell()) {

You could reuse that helper method that I mentioned above here.
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

r+ with comments above addressed, dunno if I screwed up something in the mozreview.
Attachment #8592353 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

/r/7043 - Bug 1152479 - Extract the implementations of layout-related nsIDOMWindowUtils APIs used by APZ into nsLayoutUtils. r=ehsan
/r/7045 - Bug 1152479 - Do not use layout-related nsIDOMWindowUtils APIs from C++ APZ code. r=kats
/r/7047 - Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan

Pull down these commits:

hg pull -r 33e3ce1ae558eba912aad4487ff40d691ca4368e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592353 - Flags: review+ → review?(bugmail.mozilla)
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

Addressed Kats' comments. Carrying r+ for Kats.
Attachment #8592353 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

/r/7043 - Bug 1152479 - Extract the implementations of nsIDOMWindowUtils::Send{KeyEvent,MouseEvent} into nsContentUtils. r=ehsan
/r/7045 - Bug 1152479 - In C++ APZ code, use nsContentUtils rather than nsIDOMWindowUtils to send key and mouse events. r=kats

Pull down these commits:

hg pull -r 87356526e34b44658d0917d836df83e35b291e67 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592353 - Flags: review?(ehsan)
Attachment #8592353 - Flags: review?(bugmail.mozilla)
Attachment #8592353 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #12)
> There remain a couple of nsIDOMWindowUtils APIs in C++ APZ code
> (sendKeyEvent and sendMouseEvent), which I'll transition in a separate
> patch, probably to nsContentUtils.

This is done by the new patches.
Hmm, I didn't intend to remove/obsolete the first MozReview request - the original patches are still there...
Attachment #8592353 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

https://reviewboard.mozilla.org/r/7041/#review5889

::: dom/base/nsContentUtils.cpp
(Diff revision 4)
> +nsContentUtils::GetWidget(nsCOMPtr<nsIPresShell> aPresShell, nsPoint* aOffset) {

Please make this an nsIPresShell*.
Attachment #8592353 - Flags: review?(ehsan)
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

/r/7043 - Bug 1152479 - Extract the implementations of nsIDOMWindowUtils::Send{KeyEvent,MouseEvent} into nsContentUtils. r=ehsan
/r/7045 - Bug 1152479 - In C++ APZ code, use nsContentUtils rather than nsIDOMWindowUtils to send key and mouse events. r=kats

Pull down these commits:

hg pull -r c989db3f7a0a41682429a0febfa8c51f2bb99e7f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592353 - Flags: review?(ehsan)
Attachment #8592353 - Flags: review?(bugmail.mozilla)
Attachment #8592353 - Flags: review+
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

Updated to address comment 28. Carrying r+ for Kats.
Attachment #8592353 - Flags: review?(bugmail.mozilla) → review+
Attachment #8592353 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/46d9e7687722
https://hg.mozilla.org/mozilla-central/rev/786b564f6e0a
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
As with bug 1039818, I will not request uplift for this unless it's demonstrated to cause a blocker bug.
Attachment #8592353 - Attachment is obsolete: true
Attachment #8619991 - Flags: review+
Attachment #8619992 - Flags: review+
Attachment #8619993 - Flags: review+
Attachment #8619994 - Flags: review+
Depends on: 1175442
You need to log in before you can comment on or make changes to this bug.