script blockers and similar techniques should be avoided
L. David Baron
http://hg.mozilla.org/mozilla-central/rev/a05453237290 ) were
introduced to prevent a specific type of serious bug that could be
present in *uncommon* cases. However, the technique (which is also
shared by the older IsSafeToFlush methods) has a serious risk of
fixing bugs in one part of code only at the expense of causing them
elsewhere. Because of this, I think we should avoid introducing new
uses of this technique unless necessary (and definitely avoid
introducing it to prevent problems in *common* cases), and we should
try to reduce our existing uses.
More generally, the type of technique I'm worried about is where a
function (A) relies on a certain data structure not changing across
a call that it makes to some other function, and to ensure that it
doesn't change, sets global state to say that certain APIs (B) that
normally change that data structure will simply be ignored.
This has the risk that some function (M) along the path from A to B,
more directly responsible for calling B, was actually relying on B
to do its usual work. Ignoring the API call to B could cause bugs
in that intermediate function M. For example, script blockers
prevent FlushPendingNotifications from doing any work; this means
any callers (M) that rely on FlushPendingNotifications causing
certain data to become up-to-date will be buggy (as the price of
fixing a bug in A).
This was originally done only in cases where the call chain from A
to B was unusual (e.g., something that only happened with a
particular unusual combination of features and/or extensions), and
where there weren't any steps within that call chain that we could
break or make asynchronous without breaking more important features.
I think we should limit the use of script blockers and similar
techniques to such cases, and avoid adding them to solve common
problems.
Instead of adding script blockers to solve common problems, we
should ensure that if A relies on certain data structures not
changing, that it doesn't call functions that can change those data
structures. We can do this using both static analysis and using
assertions (which we can widen to cause assertions if things that
might call B are called within A, rather than only asserting in the
case where we actually call B within A).
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Neil
>However, the technique (which is also shared by the older IsSafeToFlush methods) has a serious risk of fixing bugs in one part of code only at the expense of causing them elsewhere.
>
Bug 519049 is an example of this, with the case of IsSafeToRunScript.
--
Warning: May contain traces of nuts.
Boris Zbarsky
> More generally, the type of technique I'm worried about is where a
> function (A) relies on a certain data structure not changing across
> a call that it makes to some other function, and to ensure that it
> doesn't change, sets global state to say that certain APIs (B) that
> normally change that data structure will simply be ignored.
This is one current use of script blockers. The other use, and the one
that most script blockers are actually for, is that of some code wanting
to do something when it's "safe" but not fully asynchronously, and in
particular synchronously from the viewpoint of page JS (that is, before
control returns to page JS, if any is running right now). This what we
use scriptrunners for, and those have worked very well to solve a number
of longstanding issues we had.
Of course having scriptrunners work safely means that anything that
can't handle script running during it really needs to set a
scriptblocker, so its callees can use scriptrunners without problems.
Perhaps we should try to distinguish these two cases somehow, or come up
with a different solution to the problem scriptrunners solve?
-Boris
Jonas Sicking
> More generally, the type of technique I'm worried about is where a
> function (A) relies on a certain data structure not changing across
> a call that it makes to some other function, and to ensure that it
> doesn't change, sets global state to say that certain APIs (B) that
> normally change that data structure will simply be ignored.
>
> This has the risk that some function (M) along the path from A to B,
> more directly responsible for calling B, was actually relying on B
> to do its usual work. Ignoring the API call to B could cause bugs
> in that intermediate function M. For example, script blockers
> prevent FlushPendingNotifications from doing any work; this means
> any callers (M) that rely on FlushPendingNotifications causing
> certain data to become up-to-date will be buggy (as the price of
> fixing a bug in A).
I think FlushPendingNotifications is a special case here. More below.
> This was originally done only in cases where the call chain from A
> to B was unusual (e.g., something that only happened with a
> particular unusual combination of features and/or extensions), and
> where there weren't any steps within that call chain that we could
> break or make asynchronous without breaking more important features.
> I think we should limit the use of script blockers and similar
> techniques to such cases, and avoid adding them to solve common
> problems.
>
> Instead of adding script blockers to solve common problems, we
> should ensure that if A relies on certain data structures not
> changing, that it doesn't call functions that can change those data
> structures. We can do this using both static analysis and using
> assertions (which we can widen to cause assertions if things that
> might call B are called within A, rather than only asserting in the
> case where we actually call B within A).
I originally introduced scriptblockers not as a way to prevent content
scripts from running, but delay their running to a point when it was
safe. I.e. when a <script> element was inserted, we didn't want to run
its script in the middle of BindToTree, but we also didn't want to block
it entirely.
FlushPendingNotifications is an exception here since it's one of the few
features that we're blocking, even though it does a lot more than
running content scripts.
However once we turn on the HTML5 parser, which removes content
notification batching, we should be able to let
FlushPendingNotifications run even when there are active script blockers.
So I guess my take is that instead of using script blockers in fewer
places, make script blockers be more selective about what they block.
/ Jonas
Boris Zbarsky
> However once we turn on the HTML5 parser, which removes content
> notification batching, we should be able to let
> FlushPendingNotifications run even when there are active script blockers.
Would we? I think we rely on that being blocked in various places
now.... In particular, flushing reflow can run script at the moment, no?
-Boris
Boris Zbarsky
> Would we? I think we rely on that being blocked in various places
> now.... In particular, flushing reflow can run script at the moment, no?
And flushing restyles too, due to XBL constructors.
-Boris
Jonas Sicking
My understanding is that flushing reflows can only run script because
flushing reflows, flushes content notifications, which in turn can run
script. If we didn't need to flush content notifications (which I
understand to be the case in the HTML5 parser), then flushing reflows
can't run script.
As for XBL constructors, I think we should be able to run those off a
scriptrunner.
/ Jonas
Boris Zbarsky
> My understanding is that flushing reflows can only run script because
> flushing reflows, flushes content notifications, which in turn can run
> script.
This is false, last I checked. In particular, flushing reflows can run
reflow callbacks, which can include changes to DOM attributes (e.g. on
scrollbars). Again, last I checked.
> As for XBL constructors, I think we should be able to run those off a
> scriptrunner.
Yes, but how does that help callers who don't have a script blocker
around the flush? And for the ones that do, that means the layout won't
take the XBL constructors into account, right?
If we're ok with that, and if we can confirm that restyle processing
can't trigger script itself, then we might be fine here for callers that
don't expect frames to live across the flush. Those would need to be
changed.
-Boris
L. David Baron
What about fixing things the other way around?
Can we avoid flushing in places where it's not safe to run script?
(If so, can we make doing so assert?)
Boris Zbarsky
> Can we avoid flushing in places where it's not safe to run script?
In theory, yes. That requires a good bit of analysis of all the
callsites that can lead to a flush, and possibly annotating the
functions that can lead to a flush for use in static analysis. Might be
a lot of work.
-Boris
Jonas Sicking
> On 1/11/10 3:30 PM, Jonas Sicking wrote:
>> My understanding is that flushing reflows can only run script because
>> flushing reflows, flushes content notifications, which in turn can run
>> script.
>
> This is false, last I checked. In particular, flushing reflows can run
> reflow callbacks, which can include changes to DOM attributes (e.g. on
> scrollbars). Again, last I checked.
>
>> As for XBL constructors, I think we should be able to run those off a
>> scriptrunner.
>
> Yes, but how does that help callers who don't have a script blocker
> around the flush?
I can see two solutions. Either we require people that flush to first
put a scriptblocker on the stack. Or we do these things asynchronously
if there aren't script blockers on the stack.
> And for the ones that do, that means the layout won't
> take the XBL constructors into account, right?
You mean that if an XBL ctor modifies the DOM, then that won't happen
until later?
/ Jonas
Boris Zbarsky
>> And for the ones that do, that means the layout won't
>> take the XBL constructors into account, right?
>
> You mean that if an XBL ctor modifies the DOM, then that won't happen
> until later?
Right. And in particular if you flush layout and that triggers XBL
constructors, you would expect the layout to be updated to whatever
those constructors do... but if they only fire after the flush is done,
that won't be the case.
-Boris
Jonas Sicking
I've always thought of flushing as bringing layout in sync with the
current DOM. Not do things like fire outstanding events and run XBL
constructors.
Is there any other outstanding scripts/events that are run on a flush?
/ Jonas
Boris Zbarsky
> I've always thought of flushing as bringing layout in sync with the
> current DOM. Not do things like fire outstanding events and run XBL
> constructors.
That's true, but due to the way we bind XBL, the frame construction is
what creates the bindings and enqueues the ctors. So as long as we have
lazy frame construction (via restyles) that needs flushing, and as long
as the expectation is that flushing out layout should match the state
the DOM+layout would be in if there were no laziness involved we need to
run those xbl ctors.
If we stop attaching XBL from inside layout, so much the better.
> Is there any other outstanding scripts/events that are run on a flush?
Hmm. It's possible that JS-implemented tree views might be called into.
We definitely change attributes off a flush (scrollbars), but I think
untrusted can't listen to those changes. Any relevant scrollbar binding
js would run, of course.
Other than those... I _think_ there's nothing else.
-Boris
Jonas Sicking
> On 3/10/10 1:53 PM, Jonas Sicking wrote:
>> I've always thought of flushing as bringing layout in sync with the
>> current DOM. Not do things like fire outstanding events and run XBL
>> constructors.
>
> That's true, but due to the way we bind XBL, the frame construction is
> what creates the bindings and enqueues the ctors. So as long as we have
> lazy frame construction (via restyles) that needs flushing, and as long
> as the expectation is that flushing out layout should match the state
> the DOM+layout would be in if there were no laziness involved we need to
> run those xbl ctors.
Ensuring XBL is attached in the face of lazy frame construction does
seem like a problem. However I don't see how that is related to running
XBL ctors. Or to scriptblockers in general.
/ Jonas
Boris Zbarsky
>> That's true, but due to the way we bind XBL, the frame construction is
>> what creates the bindings and enqueues the ctors. So as long as we have
>> lazy frame construction (via restyles) that needs flushing, and as long
>> as the expectation is that flushing out layout should match the state
>> the DOM+layout would be in if there were no laziness involved we need to
>> run those xbl ctors.
>
> Ensuring XBL is attached in the face of lazy frame construction does
> seem like a problem. However I don't see how that is related to running
> XBL ctors.
If someone asks for "the width of this element", we need to flush out
all work we have buffered up that affects that width. That means frame
construction, binding attachment if those frames attach bindings,
constructors for those bindings (because they can change the width!) if
they have any, and reflow.
Which is to say that a layout flush needs to run xbl ctors.
-Boris
Jonas Sicking
I'm still unconvinced that we need to flush XBL ctors. All else I agree
with. Why group XBL ctors together with frame construction rather with,
for example, resize events, which can also affect the width of an element?
Ideally people that care about the size of an element update themselves
when the size of that element changes. That could should catch whatever
the XBL ctor does when it runs.
I think right now we are taking quite a bit of pain due to the fact that
layout flushes must only happen at safe times, if we assume the ideal
case above (and fix the cases that doesn't follow that, if there are
any), I think that would simplify our code.
/ Jonas
Boris Zbarsky
> I'm still unconvinced that we need to flush XBL ctors. All else I agree
> with. Why group XBL ctors together with frame construction rather with,
> for example, resize events, which can also affect the width of an element?
Because XBL attachment in chrome is supposed to be synchronous, and we
violate that with the async frame construction....
> Ideally people that care about the size of an element update themselves
> when the size of that element changes. That could should catch whatever
> the XBL ctor does when it runs.
In practice, I believe chrome does not in fact do that.
> I think right now we are taking quite a bit of pain due to the fact that
> layout flushes must only happen at safe times, if we assume the ideal
> case above (and fix the cases that doesn't follow that, if there are
> any), I think that would simplify our code.
I'm all in favor of making layout flushes safe, sure.
-Boris