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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack

STR:

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

Noticed when trying to debug bug 1412743.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 2

[Boris Zbarsky [: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...

Comment 3

[Jan Henning [:JanH]]

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Boy, I'm looking at the initialization of the pres context / shell / similar stuff, and it's such a mess :(

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make font-inflation computation less lazy.

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.

Comment 6

[Jan Henning [:JanH]]

(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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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

Comment 8

[Jan Henning [:JanH]]

(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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 12

[Jan Henning [:JanH]]

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Updated patch

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?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I'm debugging it too btw, I don't see why my patch would necessarily change behavior...

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 18

[Daniel Veditz [:dveditz]]

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.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch.

Ok, this actually does the work... Note that GetViewportInfo() already relied on the doctype not changing, so dropping that should be fine.

Comment 20

[Jan Henning [:JanH]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Updated per comments.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ce74c61c5c3

How far back does this issue go?

Comment 27

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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.