Assertion failure nsContentUtils::IsSafeToRunScript() with non-default systemFontScale

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

({sec-moderate})

unspecified
mozilla58
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(2 attachments, 3 obsolete attachments)

Posted file Stack
STR:

 * On a debug build, toggle font.size.systemFontScale to, e.g., 150.
 * Load any page.

Noticed when trying to debug bug 1412743.
This can actually be a security hazard I believe...

I think we may be able to create a shell after script has run, and it could run media query listeners and do all sort of badness... Boris, is that right? How prioritary should this be, given this requires a pref to get set to a non-default value?
Flags: needinfo?(bzbarsky)
> This can actually be a security hazard I believe...

Yes.  Based on the stack, we're in the middle of frame construction.  Running script in here would be pretty bad.

I think it's entirely non-obvious that nsIPresShell::FontSizeInflationEnabled should be able to have side-effects like this....

We should probably put all that stuff off on a scriptrunner instead of asserting, since clearly people can in fact reach this code from all over.

> How prioritary should this be, given this requires a pref to get set to a non-default value?

Looks like there's UI on Firefox for Android to toggle this pref, so probably somewhat of a priority...
Group: layout-core-security
Flags: needinfo?(bzbarsky) → needinfo?(jh+bugzilla)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> > How prioritary should this be, given this requires a pref to get set to a non-default value?
> 
> Looks like there's UI on Firefox for Android to toggle this pref, so
> probably somewhat of a priority...

Yes, it's what used behind the scenes for Accessibility -> Use system font size. If that is flipped on (it's off by default), then the systemFontScale pref is set to whatever corresponding font scale value Android itself is using.
Flags: needinfo?(jh+bugzilla)
Boy, I'm looking at the initialization of the pres context / shell / similar stuff, and it's such a mess :(
This makes it a bit more straight-forward to change the system font scale,
preserving the sync MediaFeatureChanged event.

This also avoids notifying media queries when the shell is not initialized.

In particular, the patch in bug 1404545 allows calling MediaFeatureValuesChanged
on a still-initializing pres-shell. This is nasty, and all this initialization
order is kind of a mess, but I'm not reworking it for now...

Also, this drops the invalidation of font-inflation when a doctype is added to
the document. I see nothing in there that would depend on doctype, and if it was
we'd need to also do all the work we do in nsLayoutUtils.
Attachment #8924148 - Flags: review?(bzbarsky)
Assignee

Updated

2 years ago
Assignee: nobody → emilio
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Also, this drops the invalidation of font-inflation when a doctype is added
> to the document. I see nothing in there that would depend on doctype, and if it
> was we'd need to also do all the work we do in nsLayoutUtils.

Actually there is, hiding behind the call to GetViewportInfo [1].

The document type then matters here [2] as part of the logic to decide whether the page counts as mobile-friendly (and therefore doesn't need font inflation) or not.
In practice I'd guess that today <meta name="viewport" ...> [3] is the more relevant way of detecting a mobile-friendly page, but I've got no idea whether that's really the case.




[1] https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/layout/base/PresShell.cpp#10775
[2] https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/dom/base/nsDocument.cpp#8225
[3] https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/dom/base/nsDocument.cpp#8293-8304
(In reply to Jan Henning [:JanH] from comment #6)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> > Also, this drops the invalidation of font-inflation when a doctype is added
> > to the document. I see nothing in there that would depend on doctype, and if it
> > was we'd need to also do all the work we do in nsLayoutUtils.
> 
> Actually there is, hiding behind the call to GetViewportInfo [1].
> 
> The document type then matters here [2] as part of the logic to decide
> whether the page counts as mobile-friendly (and therefore doesn't need font
> inflation) or not.
> In practice I'd guess that today <meta name="viewport" ...> [3] is the more
> relevant way of detecting a mobile-friendly page, but I've got no idea
> whether that's really the case.

Hmm, that's a good point (and somewhat annoying I guess). I had gone through that method, but overlooked that...

I guess we would be relying on the reflow posted from UpdateEffectiveTextZoom? Otherwise there's nothing reflowing the page due to the font inflation change per se... It's not clear to me the previous code would work, and of course no test caught this...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> I guess we would be relying on the reflow posted from
> UpdateEffectiveTextZoom? Otherwise there's nothing reflowing the page due to
> the font inflation change per se... It's not clear to me the previous code
> would work, and of course no test caught this...

Even today font inflation can be enabled *without* changing the systemFontScale (which happens if you flip the pref on Android when your phone's font size is set to "Normal"), and of course compared to font inflation this is a very recent feature.

Maybe it's relying on the fact that
- changing one of Firefox's font-inflation related settings (font.size.inflation.minTwips etc.) is expected to take effect only after reloading the page
- the document type and meta tags are normally known early enough during a page load to take effect [1][2]

[1] Putting a breakpoint on PresShell::ContentInserted for a normal page load, it seems to be called while mDidInitialize = false, i.e. while the function will simply return early and do nothing anyway. But I've got no idea if that's guaranteed to be always the case and what would have been the expected behaviour if the document type is changed dynamically after the page has loaded, which I guess is when ContentInserted/Removed might become relevant.

[2] As for the <meta name="viewport" ...> tags, those are added *after* the font inflation state has been computed for the first time [3], but when the DOMMetaAdded event comes through, the MobileViewportManager might trigger a reflow somehow when setting the changed viewport through [4]

[3] Which means that, provided one of the font.size.inflation settings is enabled, mFontSizeInflation is momentarily calculated as being true, before then reverting to false if one of the corresponding viewport meta tags has been detected.

[4] https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/layout/base/nsLayoutUtils.cpp#9128
(In reply to Jan Henning [:JanH] from comment #8)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > I guess we would be relying on the reflow posted from
> > UpdateEffectiveTextZoom? Otherwise there's nothing reflowing the page due to
> > the font inflation change per se... It's not clear to me the previous code
> > would work, and of course no test caught this...

Right, I don't think this is something that we should spend a lot of effort to support, and I think it's broken per the comment 7. I'm not sure I want to spend tons of time on it tbh, but if you think differently I may just post a scriptrunner or something to update it.

> [2] As for the <meta name="viewport" ...> tags, those are added *after* the
> font inflation state has been computed for the first time [3], but when the
> DOMMetaAdded event comes through, the MobileViewportManager might trigger a
> reflow somehow when setting the changed viewport through [4]

Right, that matches my understanding of it.

> [3] Which means that, provided one of the font.size.inflation settings is
> enabled, mFontSizeInflation is momentarily calculated as being true, before
> then reverting to false if one of the corresponding viewport meta tags has
> been detected.

Yeah, but that's handled just fine, both before and after my patch, I think.

Still I think the order we initialize things in general doesn't make much sense (in the sense of: we create the styleset, evaluate media queries, and _then_ set zoom and other stuff from nsDocumentViewer). That looks somewhat borked, but...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Still I think the order we initialize things in general doesn't make much
> sense (in the sense of: we create the styleset, evaluate media queries, and
> _then_ set zoom and other stuff from nsDocumentViewer). That looks somewhat
> borked, but...

Well, I guess not borked, just inefficient, and causing this sort of bugs :)
Comment on attachment 8924148 [details] [diff] [review]
Make font-inflation computation less lazy.

This fails a couple font inflation reftests, looking into those, but cancelling review for now.
Attachment #8924148 - Flags: review?(bzbarsky)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Right, I don't think this is something that we should spend a lot of effort
> to support, and I think it's broken per the comment 7. I'm not sure I want
> to spend tons of time on it tbh, but if you think differently I may just
> post a scriptrunner or something to update it.

Sure, if in practice it's not any worse than today then no objections (but the commit message then obviously needs updating).

> This fails a couple font inflation reftests, looking into those, but
> cancelling review for now.

Additionally, when I attempt to actually run this on my phone, it crashes in
> * thread #12, name = 'Gecko', stop reason = signal SIGSEGV: invalid address (fault address: 0x8)
>   * frame #0: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] RefPtr<nsPresContext>::get(this=0x00000008) const at RefPtr.h:287
>     frame #1: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] RefPtr<nsPresContext>::operator nsPresContext*(this=0x00000008) const & at RefPtr.h:300
>     frame #2: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] nsIPresShell::GetPresContext(this=0x00000000) const at nsIPresShell.h:255
>     frame #3: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(this=0x8e434800, aDisplaySize=0xa082c990) at nsDocument.cpp:8177
>     frame #4: 0x9d6acbfc libxul.so`nsIPresShell::DetermineFontSizeInflationState(this=0x8dfe5000) at PresShell.cpp:10770
>     frame #5: 0x9d6a2a8a libxul.so`nsIPresShell::RecomputeFontSizeInflationEnabled(this=0x8dfe5000) at PresShell.cpp:10695
>     frame #6: 0x9d6a2890 libxul.so`mozilla::PresShell::Init(this=0x8dfe5000, aDocument=<unavailable>, aPresContext=0x8df43400, aViewManager=<unavailable>, aStyleSet=<unavailable>) at PresShell.cpp:1017
>     frame #7: 0x9cd14864 libxul.so`nsDocument::CreateShell(this=0x8e434800, aContext=0x8df43400, aViewManager=0x8dfcc040, aStyleSet=StyleSetHandle @ 0xa082ca7c) at nsDocument.cpp:4147
>     [...]
(In reply to Jan Henning [:JanH] from comment #12)
> Additionally, when I attempt to actually run this on my phone, it crashes in
> > * thread #12, name = 'Gecko', stop reason = signal SIGSEGV: invalid address (fault address: 0x8)
> >   * frame #0: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] RefPtr<nsPresContext>::get(this=0x00000008) const at RefPtr.h:287
> >     frame #1: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] RefPtr<nsPresContext>::operator nsPresContext*(this=0x00000008) const & at RefPtr.h:300
> >     frame #2: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [inlined] nsIPresShell::GetPresContext(this=0x00000000) const at nsIPresShell.h:255
> >     frame #3: 0x9cd1a2da libxul.so`nsDocument::GetViewportInfo(this=0x8e434800, aDisplaySize=0xa082c990) at nsDocument.cpp:8177
> >     frame #4: 0x9d6acbfc libxul.so`nsIPresShell::DetermineFontSizeInflationState(this=0x8dfe5000) at PresShell.cpp:10770
> >     frame #5: 0x9d6a2a8a libxul.so`nsIPresShell::RecomputeFontSizeInflationEnabled(this=0x8dfe5000) at PresShell.cpp:10695
> >     frame #6: 0x9d6a2890 libxul.so`mozilla::PresShell::Init(this=0x8dfe5000, aDocument=<unavailable>, aPresContext=0x8df43400, aViewManager=<unavailable>, aStyleSet=<unavailable>) at PresShell.cpp:1017
> >     frame #7: 0x9cd14864 libxul.so`nsDocument::CreateShell(this=0x8e434800, aContext=0x8df43400, aViewManager=0x8dfcc040, aStyleSet=StyleSetHandle @ 0xa082ca7c) at nsDocument.cpp:4147
> >     [...]

Yeah, that matches the failures I'm seeing, and it's because from PresShell::Init Document::mPresShell still doesn't point to it... So I'll figure out what to do.
> But I've got no idea if that's guaranteed to be always the case

Probably not, especially where document.open/write is involved (such that the caller can explicitly control the ordering of parsing bits and layout flushes).

> and what would have been the expected behaviour if the document type is changed dynamically after the page
> has loaded

Yeah, who knows.  In practice, no one outside of crazy tests is likely to dynamically inject a doctype after some elements are in the document....
Posted patch Updated patch (obsolete) — Splinter Review
So with this patch the tests that fail are the disable-font-infl-on-mobile.html ones.

However, I have no idea about what that test is testing... and I suspect that it was relying on a bug that is fixed with my patch.

In particular, since bug 803908, those tests force font-inflation to be on, so I can't see why wouldn't force-inflation be enabled...

Am I missing something?
Flags: needinfo?(jh+bugzilla)
Attachment #8924628 - Flags: feedback?(jh+bugzilla)
I'm debugging it too btw, I don't see why my patch would necessarily change behavior...
Assignee

Updated

2 years ago
Attachment #8924148 - Attachment is obsolete: true
Yeah, I was missing something, which is the doctype stuff... This code now runs before the doctype has been added to the document, so we can't look at it... Sigh.
Flags: needinfo?(jh+bugzilla)
Calling this sec-moderate because it sounds like a non-default pref setting has to be used. Could be worse if that's a common setting, though.
Keywords: sec-moderate
Posted patch Patch. (obsolete) — Splinter Review
Ok, this actually does the work... Note that GetViewportInfo() already relied on the doctype not changing, so dropping that should be fine.
Attachment #8924628 - Attachment is obsolete: true
Attachment #8924628 - Flags: feedback?(jh+bugzilla)
Attachment #8924653 - Flags: review?(jh+bugzilla)
Attachment #8924653 - Flags: review?(bzbarsky)
Comment on attachment 8924653 [details] [diff] [review]
Patch.

Review of attachment 8924653 [details] [diff] [review]:
-----------------------------------------------------------------

I'll leave the final judgement to Boris, but this looks okay as far as the font inflation logic is concerned and I'm fine with dropping the dynamic doctype handling.

::: layout/base/PresShell.cpp
@@ +1013,5 @@
> +  mFontSizeInflationMinTwips = nsLayoutUtils::FontSizeInflationMinTwips();
> +  mFontSizeInflationLineThreshold = nsLayoutUtils::FontSizeInflationLineThreshold();
> +  mFontSizeInflationForceEnabled = nsLayoutUtils::FontSizeInflationForceEnabled();
> +  mFontSizeInflationDisabledInMasterProcess = nsLayoutUtils::FontSizeInflationDisabledInMasterProcess();
> +  // We'll compute font-size inflation on Initialize(), when we know the

Small tweak: "We'll compute the font size inflation state in Initialize(), when ..." etc.
Attachment #8924653 - Flags: review?(jh+bugzilla) → review+
Comment on attachment 8924653 [details] [diff] [review]
Patch.

>@@ -1725,6 +1730,9 @@ PresShell::Initialize(nscoord aWidth, nscoord aHeight)
>+  RecomputeFontSizeInflationEnabled();

OK.  So this is being called after mDidInitialize is set to true.  It can proceed to call SetSystemFontScale() on the prescontext, which ill call nsPresContext::UpdateEffectiveTextZoom.  That calls MediaFeatureValuesChanged(), which looks to me like it can run script (by notifying media query listeners).  By this point mShell->DidInitialize() is true, so we can reach that code.

Then we unwind, but we (and more importantly rootFrame, which we then proceed to use) might be dead by then, no?

>+++ b/layout/base/nsPresContext.cpp
>   mPendingViewportChange = false;

Why is this after the early-return for non-initialized-presshell?
Attachment #8924653 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21)
> Comment on attachment 8924653 [details] [diff] [review]
> Patch.
> 
> >@@ -1725,6 +1730,9 @@ PresShell::Initialize(nscoord aWidth, nscoord aHeight)
> >+  RecomputeFontSizeInflationEnabled();
> 
> OK.  So this is being called after mDidInitialize is set to true.  It can
> proceed to call SetSystemFontScale() on the prescontext, which ill call
> nsPresContext::UpdateEffectiveTextZoom.  That calls
> MediaFeatureValuesChanged(), which looks to me like it can run script (by
> notifying media query listeners).  By this point mShell->DidInitialize() is
> true, so we can reach that code.

I doubted about whether to put this before setting mDidInitialize... But then I suspected that it was fine if that method called into JS, since we didn't enforce anything there, and we used nsAutoScriptBlocker for frame construction

> Then we unwind, but we (and more importantly rootFrame, which we then
> proceed to use) might be dead by then, no?

Well, you're right on this one, that this has no reason to be under the rootFrame assignment. Though ideally rootFrame should always be null... I guess it's not if somehow the listeners trigger a flush or what not, should I just put it before setting mDidInitialize? I'll submit a patch just doing that.

> 
> >+++ b/layout/base/nsPresContext.cpp
> >   mPendingViewportChange = false;
> 
> Why is this after the early-return for non-initialized-presshell?

Right, no need to indeed.
Attachment #8925135 - Flags: review?(bzbarsky)
Assignee

Updated

2 years ago
Attachment #8924653 - Attachment is obsolete: true
Comment on attachment 8925135 [details] [diff] [review]
Updated per comments.

>+  RecomputeFontSizeInflationEnabled();

Assert that !mIsDestroying after this, because we should not have run script?

Worth making sure we have tests for someone registering a media query listener while an iframe is display:none and then it changing to non-none display.
Attachment #8925135 - Flags: review?(bzbarsky) → review+
I added the assertion, and also uncommented the skip-if(isDebugBuild) that I added in bug 1412743.

remote: View your change here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce74c61c5c3355689b65ca7453020e90897ee00
https://hg.mozilla.org/mozilla-central/rev/1ce74c61c5c3

How far back does this issue go?
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(emilio)
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Probably as far as bug 862763... Not sure whether it's worth uplifting, it's not a particularly trivial patch, so at least I'd wait a couple days just in case.
Flags: needinfo?(emilio)
Group: layout-core-security → core-security-release
Given that it's a sec-moderate and the patch is decent size, I think letting this ride the 58 train is the correct choice.
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.