Closed Bug 383331 Opened 13 years ago Closed 12 years ago

[FIX]Flash of unstyled content possible if subframe asks for its size


(Core :: DOM: Core & HTML, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(Keywords: regression)


(1 file)

I saw this on but it depends on the exact google ad they had there...

What's basically happening is that a subframe (google ad in this case) runs script, which gets the innerHeight of the subframe window, which forces out layout of the parent document, even though the sheet for the parent document is still loading.

I think the simplest thing to do is to block script in all subframes when we block it for sheets.  The other option is to have the scriptloader recursively block all subframes when it's blocked, but I'm not sure we want this in general.
Flags: in-testsuite?
Flags: blocking1.9?
Though... if we just block subframes when we see the sheet, that won't help with subframes that are parsed _after_ the sheet has started loading. Need a better approach somehow, where newly-added script loaders are blocked if their ancestor is blocked, and get unblocked when it gets unblocked.  Or something.
Hmm.. for whatever reason I at first thought this was XML related, but it can happen just as easily in HTML right?

Blocking all scriptloaders in the current frame tree sounds sort of painful, especially if frames are added and removed while we're parsing, which could happen from another window.
Yeah, this is not XML-specific at all.

And yes, dealing with frames being added/removed is a bit of a pain.

Perhaps what we could do is script loaders could check whether all ancestors are unblocked before running script?  And it they're not, post an event to do it or something?
Wouldn't we need it to be a timer rather than just an event, since we're waiting for network traffic to finish. Though it seems sort of suboptimal (==arbitrary) to be running scripts off of timers.
An event would get there eventually, but yes a timer might be better.

Even better would be a setup where the child scriptloader could ask the parent explicitly to notify it when the parent gets unblocked?
Yeah, that would be a much better way of doing it. As long as we can unregister if our document is removed as child document.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
For what it's worth, I've been seeing this a good bit on sites with Google ads... That's why I filed the bug.  :(
Ok, renominating and we'll see what we can do.
Flags: blocking1.9- → blocking1.9?
Whiteboard: [wanted-1.9]
I'll try to come up with something in the next few days, but if I don't get to it then....  :(
I guess this is also the reason that I'm seeing the flash of unstyled content at:
No.  The reason there is that they open the <body> before any of their stylesheets has started loading (by putting a <br> inside the <head>)....

Though they might indeed hit this bug if they were to fix that.
Ok, would be great if we could get this fixed. Alternatively we could get google to not force a reflow if they are the major cause of this.

Another solution that I've been thinking of is if we fix the whole flushnotification mess, we might be able to hold of appending any nodes between we start parsing a stylesheet until we're done parsing it. Or something :)
Flags: blocking1.9? → blocking1.9+
Can fix this after the beta ships.  This won't affect daily usage.  Scheduling for b2 per Boris.
Target Milestone: --- → mozilla1.9beta2
Depends on: 396226
Priority: -- → P1
Summary: Flash of unstyled content possible if subframe asks for its size → [FIX]Flash of unstyled content possible if subframe asks for its size
Attached patch Proposed fixSplinter Review
Attachment #280950 - Flags: superreview?(jonas)
Attachment #280950 - Flags: review?(jonas)
Target Milestone: mozilla1.9 M9 → ---
Comment on attachment 280950 [details] [diff] [review]
Proposed fix

> nsScriptLoader::ProcessPendingRequests()
> {
>   nsRefPtr<nsScriptLoadRequest> request;
>+  // Important: the ReadyToExecuteScripts() check needs to come before the
>+  // Count() check so that we can get blocked on an ancestor even if we have no
>+  // pending requests.

Is this really true? Won't we simply bail if Count() is zero and get blocked next time we end up in this function?

>   while (ReadyToExecuteScripts() && mPendingRequests.Count() &&
>          !(request = mPendingRequests[0])->mLoading) {
>     mPendingRequests.RemoveObjectAt(0);
>     ProcessRequest(request);
>   }
>+  if (SelfReadyToExecuteScripts()) {
>+    while (ReadyToExecuteScripts() && !mPendingChildLoaders.IsEmpty()) {

The Self...() check seems unnecessary as ReadyToExecuteScripts will check exactly that the first thing it does.

r/sr=me with that fixed/answered.
Attachment #280950 - Flags: superreview?(jonas)
Attachment #280950 - Flags: superreview+
Attachment #280950 - Flags: review?(jonas)
Attachment #280950 - Flags: review+
Attachment #280950 - Flags: approval1.9+
> Won't we simply bail if Count() is zero 

Hmm...  Yes.  I'm not sure why I wrote that comment now.  I'll remove it.

> The Self...() check seems unnecessary as ReadyToExecuteScripts will check
> exactly that the first thing it does.

Ah, good point.  I wanted to make sure we didn't block twice on the parent, but it should be OK.  I'll remove this too (and add a comment in nsScriptLoader::ReadyToExecuteScripts).
Checked in.  Waiting on test infrastructure to write the test.
Closed: 12 years ago
Resolution: --- → FIXED
I'm still seeing the issue in comment 10.
I'm a bit confused, comment 11 suggests this is the fault of the site, but this isn't happening on branch. 
Comment 11 just explains why it happens.  It's not really the "fault" of the site, except insofar as their HTML is invalid in a silly way.  That site should hopefully get fixed by bug 387669.  I'll try to test on it as well.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.