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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

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

Comment 1

[Botond Ballo [:botond]]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace

Here's the backtrace that shows how it's happening, taken from the above try run.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace

Copy/paste fail

Comment 4

[Botond Ballo [:botond]]

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 6

[Botond Ballo [:botond]]

(Ehsan: no reponse required, it'd just be a shame if you missed comment 5 :))

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 9

[Botond Ballo [:botond]]

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.

Comment 10

[Botond Ballo [:botond]]

Have some in-progress patches that stop using nsIDOMWindowUtils from APZ-related C++ code.

Comment 11

[Botond Ballo [:botond]]

Attachment: 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/

Comment 12

[Botond Ballo [:botond]]

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 13

[Botond Ballo [:botond]]

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/

Comment 14

[Botond Ballo [:botond]]

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.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

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 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

r+ with comments above addressed, dunno if I screwed up something in the mozreview.

Comment 17

[Botond Ballo [:botond]]

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/

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

Addressed Kats' comments. Carrying r+ for Kats.

Comment 19

[(no longer active)]

https://reviewboard.mozilla.org/r/7047/#review5859

Ship It!

Comment 20

[(no longer active)]

https://reviewboard.mozilla.org/r/7047/#review5861

Ship It!

Comment 22

[Botond Ballo [:botond]]

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/

Comment 23

[Botond Ballo [:botond]]

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

Comment 24

[Botond Ballo [:botond]]

Hmm, I didn't intend to remove/obsolete the first MozReview request - the original patches are still there...

Comment 25

[Botond Ballo [:botond]]

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.

Comment 26

[Botond Ballo [:botond]]

Try push for the whole thing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe83dad964f

Comment 27

[Kartikaya Gupta (email:[e-mail])]

https://reviewboard.mozilla.org/r/7045/#review5883

Ship It!

Comment 28

[(no longer active)]

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

Comment 29

[Botond Ballo [:botond]]

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/

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8592353 [details]
MozReview Request: bz://1152479/botond

Updated to address comment 28. Carrying r+ for Kats.

Comment 31

[(no longer active)]

https://reviewboard.mozilla.org/r/7043/#review5905

Ship It!

Comment 32

[(no longer active)]

https://reviewboard.mozilla.org/r/7043/#review5907

Ship It!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Botond Ballo [:botond]]

Landed remaining patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/46d9e7687722
https://hg.mozilla.org/integration/mozilla-inbound/rev/786b564f6e0a

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46d9e7687722
https://hg.mozilla.org/mozilla-central/rev/786b564f6e0a

Comment 36

[Botond Ballo [:botond]]

As with bug 1039818, I will not request uplift for this unless it's demonstrated to cause a blocker bug.

Comment 38

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1152479 - Remove nsIDOMWindowUtils::isFirstPaint, as it only had C++ users. r=ehsan

Comment 39

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1152479 - Extract the implementations of nsIDOMWindowUtils::Send{KeyEvent,MouseEvent} into nsContentUtils. r=ehsan

Comment 40

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1152479 - In C++ APZ code, use nsContentUtils rather than nsIDOMWindowUtils to send key and mouse events. r=kats

Comment 41

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1152479 - Assert IsCallerChrome() in some layout-related nsIDOMWindowUtils APIs. r=ehsan