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

RESOLVED FIXED in Firefox 40

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla40
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 2 obsolete attachments)

32.42 KB, text/plain
Details
4.98 KB, text/plain
Details
39 bytes, text/x-review-board-request
Away for a while
: review+
botond
: review+
Details | Review
39 bytes, text/x-review-board-request
Away for a while
: review+
botond
: review+
Details | Review
39 bytes, text/x-review-board-request
Away for a while
: review+
botond
: review+
Details | Review
39 bytes, text/x-review-board-request
Away for a while
: review+
botond
: review+
Details | Review
(Assignee)

Description

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

Updated

3 years ago
Blocks: 1039818
(Assignee)

Comment 1

3 years ago
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.
Created attachment 8589866 [details]
Backtrace

Here's the backtrace that shows how it's happening, taken from the above try run.
Created attachment 8589867 [details]
Backtrace

Copy/paste fail
Attachment #8589866 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
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.
Comment hidden (spam)
(Assignee)

Comment 6

3 years ago
(Ehsan: no reponse required, it'd just be a shame if you missed comment 5 :))
Flags: needinfo?(ehsan)
Comment hidden (spam)
Created attachment 8590413 [details]
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.
(Assignee)

Comment 9

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

Updated

3 years ago
No longer blocks: 1039818
(Assignee)

Comment 10

3 years ago
Have some in-progress patches that stop using nsIDOMWindowUtils from APZ-related C++ code.
Assignee: nobody → botond
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 17

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

Comment 18

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

3 years ago
https://reviewboard.mozilla.org/r/7047/#review5859

Ship It!

Comment 20

3 years ago
https://reviewboard.mozilla.org/r/7047/#review5861

Ship It!
Comment hidden (spam)
(Assignee)

Comment 22

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

Comment 23

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

Comment 24

3 years ago
Hmm, I didn't intend to remove/obsolete the first MozReview request - the original patches are still there...
(Assignee)

Comment 25

3 years ago
Landed first set of patches: 

https://hg.mozilla.org/integration/mozilla-inbound/rev/91e5592ce4b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d34adf1d56
https://hg.mozilla.org/integration/mozilla-inbound/rev/43818b455690

Leaving bug open until the second set of patches is reviewed and lands.
Keywords: leave-open
(Assignee)

Comment 26

3 years ago
greentry
Try push for the whole thing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe83dad964f
https://reviewboard.mozilla.org/r/7045/#review5883

Ship It!
Attachment #8592353 - Flags: review?(bugmail.mozilla) → review+

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

3 years ago
https://reviewboard.mozilla.org/r/7043/#review5905

Ship It!

Comment 32

3 years ago
https://reviewboard.mozilla.org/r/7043/#review5907

Ship It!

Updated

3 years ago
Attachment #8592353 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/91e5592ce4b6
https://hg.mozilla.org/mozilla-central/rev/98d34adf1d56
https://hg.mozilla.org/mozilla-central/rev/43818b455690
(Assignee)

Comment 34

3 years ago
Landed remaining patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/46d9e7687722
https://hg.mozilla.org/integration/mozilla-inbound/rev/786b564f6e0a
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/46d9e7687722
https://hg.mozilla.org/mozilla-central/rev/786b564f6e0a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 36

3 years ago
As with bug 1039818, I will not request uplift for this unless it's demonstrated to cause a blocker bug.
(Assignee)

Comment 37

3 years ago
Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond
Attachment #8592353 - Attachment is obsolete: true
Attachment #8619991 - Flags: review+
Attachment #8619992 - Flags: review+
Attachment #8619993 - Flags: review+
Attachment #8619994 - Flags: review+
(Assignee)

Comment 38

3 years ago
Created attachment 8619991 [details]
MozReview Request: Bug 1152479 - Remove nsIDOMWindowUtils::isFirstPaint, as it only had C++ users. r=ehsan
(Assignee)

Comment 39

3 years ago
Created attachment 8619992 [details]
MozReview Request: Bug 1152479 - Extract the implementations of nsIDOMWindowUtils::Send{KeyEvent,MouseEvent} into nsContentUtils. r=ehsan
(Assignee)

Comment 40

3 years ago
Created attachment 8619993 [details]
MozReview Request: Bug 1152479 - In C++ APZ code, use nsContentUtils rather than nsIDOMWindowUtils to send key and mouse events. r=kats
(Assignee)

Comment 41

3 years ago
Created attachment 8619994 [details]
MozReview Request: Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan

Updated

2 years ago
Depends on: 1175442
You need to log in before you can comment on or make changes to this bug.