Closed
Bug 383331
Opened 17 years ago
Closed 17 years ago
[FIX]Flash of unstyled content possible if subframe asks for its size
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
|
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
sicking
:
approval1.9+
|
Details | Diff | Splinter Review |
I saw this on http://www.idealog.us/2007/05/is_anyone_getti.html 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?
|
Assignee | |
Comment 1•17 years ago
|
||
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.
|
|
Assignee | |
Comment 3•17 years ago
|
||
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.
|
|
Assignee | |
Comment 5•17 years ago
|
||
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]
|
|
Assignee | |
Comment 7•17 years ago
|
||
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]
|
|
Assignee | |
Comment 9•17 years ago
|
||
I'll try to come up with something in the next few days, but if I don't get to it then.... :(
|
||
Comment 10•17 years ago
|
||
I guess this is also the reason that I'm seeing the flash of unstyled content at:
http://security-protocols.com/2007/06/14/safari-beta-301-for-windows-security-update/
|
|
Assignee | |
Comment 11•17 years ago
|
||
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+
|
||
Comment 13•17 years ago
|
||
Can fix this after the beta ships. This won't affect daily usage. Scheduling for b2 per Boris.
Target Milestone: --- → mozilla1.9beta2
|
|
Assignee | |
Updated•17 years ago
|
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
|
|
Assignee | |
Comment 14•17 years ago
|
||
Attachment #280950 -
Flags: superreview?(jonas)
Attachment #280950 -
Flags: review?(jonas)
|
|
||
Updated•17 years ago
|
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+
|
|
Assignee | |
Comment 16•17 years ago
|
||
> 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).
|
|
Assignee | |
Comment 17•17 years ago
|
||
Checked in. Waiting on test infrastructure to write the test.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
|
|
||
Comment 18•17 years ago
|
||
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.
|
|
Assignee | |
Comment 19•17 years ago
|
||
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.
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•