Closed
Bug 1152479
Opened 9 years ago
Closed 9 years ago
Non-chrome-privileged code calls into APZCCallbackHelper::UpdateRootFrame()
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
Details
Attachments
(6 files, 2 obsolete files)
32.42 KB,
text/plain
|
Details | |
4.98 KB,
text/plain
|
Details | |
39 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
botond
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
botond
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
botond
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
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
Assignee | ||
Comment 1•9 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.
Comment 2•9 years ago
|
||
Here's the backtrace that shows how it's happening, taken from the above try run.
Assignee | ||
Comment 4•9 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•9 years ago
|
||
(Ehsan: no reponse required, it'd just be a shame if you missed comment 5 :))
Flags: needinfo?(ehsan)
Comment hidden (spam) |
Comment 8•9 years ago
|
||
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•9 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 | ||
Comment 10•9 years ago
|
||
Have some in-progress patches that stop using nsIDOMWindowUtils from APZ-related C++ code.
Assignee: nobody → botond
Assignee | ||
Comment 11•9 years ago
|
||
/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•9 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•9 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•9 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.
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
https://reviewboard.mozilla.org/r/7047/#review5859 Ship It!
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/7047/#review5861 Ship It!
Comment hidden (spam) |
Assignee | ||
Comment 22•9 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•9 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•9 years ago
|
||
Hmm, I didn't intend to remove/obsolete the first MozReview request - the original patches are still there...
Assignee | ||
Comment 25•9 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•9 years ago
|
||
green try |
Try push for the whole thing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe83dad964f
Comment 27•9 years ago
|
||
https://reviewboard.mozilla.org/r/7045/#review5883 Ship It!
Updated•9 years ago
|
Attachment #8592353 -
Flags: review?(bugmail.mozilla) → review+
Comment 28•9 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•9 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•9 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•9 years ago
|
||
https://reviewboard.mozilla.org/r/7043/#review5905 Ship It!
Comment 32•9 years ago
|
||
https://reviewboard.mozilla.org/r/7043/#review5907 Ship It!
Updated•9 years ago
|
Attachment #8592353 -
Flags: review?(ehsan) → review+
Comment 33•9 years ago
|
||
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•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 36•9 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•9 years ago
|
||
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•9 years ago
|
||
Assignee | ||
Comment 39•9 years ago
|
||
Assignee | ||
Comment 40•9 years ago
|
||
Assignee | ||
Comment 41•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•