Closed Bug 312858 Opened 19 years ago Closed 17 years ago

SVG loaded from <embed> giving CSS frame constructor errors

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: tor, Unassigned)

References

(Depends on 1 open bug,
URL
)

Details

(Whiteboard: [reanalyze after XML incremental loading]) 

Load the indicated URL and use shift-reload.  You'll see the left hand side (the
SVG) display partially and disappear.  At the same time these couple of
assertions show up in the console:

###!!! ASSERTION: Unexpected child of document element containing block:
'mDocElementContainingBlock->GetFirstChild(nsnull) == nsnull', file
/home/tor/src/moz-trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9261
Break: at file
/home/tor/src/moz-trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9261
###!!! ASSERTION: already have a child frame: 'mFrames.IsEmpty()', file
/home/tor/src/moz-trunk/mozilla/layout/generic/nsHTMLFrame.cpp, line 286
Break: at file /home/tor/src/moz-trunk/mozilla/layout/generic/nsHTMLFrame.cpp,
line 286

Loading the SVG by itself (right click on the area, select "show only this
frame") works fine.
is this a trunk-only regression?
Yes, trunk only.

This worskforme in a Firefox build pulled at MOZ_CO_DATE="Sun Oct 16 21:52:07
CDT 2005"

Did this regress since then?
OK, I just retested with a Linux Firefox build pulled at MOZ_CO_DATE="Wed Oct 19
00:30:35 CDT 2005", clean profile.  No assertions other than the known session
history one.
Bracketing the regression narrowed it down to the checkin for bug 288064 for my
tree, which went in at 19:50 CDT 16-Oct-2005.

Copy paste error from bonsai - went in at 17:50 CDT.

Hmm... That fix is definitely in my tree, and I'm not seeing the assert, as I
said...  Are there any interesting changes to the tree that's having the problem?
No, those trees were stock pulls.  Were you using shift-reload?  It seems to
load ok if it pulls from the cache.

This also seems to affect SVG in an object tag (e.g.
http://169.237.230.40/ins-peak.php?section=500RF&ins=MDD&result=PD)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051019
Firefox/1.6a1

Fresh build, no special options.  Noticed in the same time frame (Friday
2005/10/15 works, Monday 2005/10/17 does not).  Sorry, but it's not a debug
build, so don't have console output.
The constructor for nsHTMLObjectElement::nsHTMLObjectElement doesn't set  
nsObjectLoadingContent.mType (which defaults to eType_Loading).

Backing out the change to line 525 of nsObjectLoadingContent.cpp fixes things.

Ah, ok.  I managed to reproduce this.  The key is that if you actually break on
the session history assertion, this one is not hit.  Jeremy, thanks for a
testcase that doesn't involve session history!

> The constructor for nsHTMLObjectElement::nsHTMLObjectElement doesn't set  
> nsObjectLoadingContent.mType (which defaults to eType_Loading).

That's correct.  We want that.  ;)
Blocks: 288064
<sigh>.  So the sequence of events here is the following:

1)  We start the load.  We're in the LOADING state.
2)  The data starts coming in.  We transition out of the LOADING state (and our
type changes to "document").  This posts a frame reconstruct event for the
content node.
3)  We start parsing the data and building up the DOM.
4)  The frame reconstruct fires.  This recreates the subdocument frame, and
calls InitialReflow() on the presshell inside the new subdocument frame (via
nsSubDocumentFrame::ShowDocShell calling nsDocShell::SetVisibility calling
DocumentViewerImpl::Show calling DocumentViewerImpl::InitPresentationStuff).  We
construct frames for whatever part of the tree has been parsed so far.
5)  Parsing finishes, and the XML content sink notifies on all the content. 
Including content that's already been parsed on.

So the real fundamental problem is that the XML sink is not incremental and
can't deal with layout trying to happen before it's done parsing...  We have a
number of bugs on various consequences of this.

So short-term we could just stop doing the LOADING state for objects.  But you
can get the same results by setting display on the iframe, of course.  Or
suddenly making it floating while the SVG is loading.  Or anything else that
messes with the rendering while we're parsing the XML.  So we should really find
a better way to do this.  Is this the time to bite the bullet and make XML
notify on the partially-parsed document?  Or should we look for something else?
I think that the subframe call InitialReflow() on its contained document. This
is causing us badness in xslt since we end up performing initial reflow on the
source document and then crash once the result document is done and we try to
replace ths source.
mmm, maybe we need to create subdocumentframes in ObjectURIChanged even if
aNotify=false?
> I think that the subframe call InitialReflow() on its contained document.

Yes, it is.  See item 4 in comment 12.

> This is causing us badness in xslt

There's xslt involved?  I suppose if you have SVG disabled...

I would say that if there is XSLT attached to the document we should simply not
put the root element in the document until after the transform happens.  Or
something.  We don't want to be rendering a partial source document when XSLT is
involved, in any case.

> create subdocumentframes in ObjectURIChanged even if aNotify=false?

How would that help?
ah, nevermind... it wouldn't.
So i missed a word in my last comment :) It should say:

I think that the subframe *shouldn't* call InitialReflow() on its contained
document.


The reason i mentioned xslt is just like you say, we don't want to do layout of
half a source xslt document, so just teaching the xml sink to do incremental
layout isn't enough. I'm not super exited about not putting the content in the
document until it is loaded. We have to put it in there anyway before the
transformation since otherwise we wouldn't be transforming the proper document.

One idea I had back in the day was to let the subframe check if the document was
still being loaded, and if so not make the call. Though I really don't
understand why the call should ever be needed, doesn't the sink take care of that?
> I think that the subframe *shouldn't* call InitialReflow() on its contained
> document.

What if you have a display:none iframe and show it?  Shouldn't that call
InitialReflow() after it creates the presshell?  I believe this is why we're
doing it...

> I'm not super exited about not putting the content in the
> document until it is loaded.

For XSLT, I think this is exactly the thing to change.  We know up front whether
we have to XSLT, and if we do, don't put the content into the document until
right before we start the transform.  That way the transform source can never be
laid out.

> Though I really don't understand why the call should ever be needed, doesn't
> the sink take care of that?

InitialReflow is only called on presshells, not documents.  If the load finishes
in a display:none iframe (so no presshell) and then we show the iframe, where
should the InitialReflow call come from?  That's why it's needed....
Don't we need to do reflow in case style information is queried on an document
in a display:none iframe? Though I guess we could do the reflow at that time.

I'm still not convinced that it's a good idea to start reflow under the sink if
the document is being loaded. Seems like at best would be a perf problem if we
start layout too soon, and at worst cause the sink to blow?

Continued the xslt discussion in bug 182460 comment 39 since it's not really
related to this bug.
> Don't we need to do reflow in case style information is queried on an document
> in a display:none iframe? 

What style information?  Layout-dependent stuff like offset*, etc just returns 0
for nodes in such documents.  getComputedStyle throws.

> I'm still not convinced that it's a good idea to start reflow under the sink
> if the document is being loaded.

I'm not sure what you mean by "under the sink"...

Back to this bug, the other option is to not construct the subdocument frame for
loading stuff.  Or something.  For objects with a standby attribute we want to
show that instead, but almost no objects have one...
> What style information?  Layout-dependent stuff like offset*, etc just returns 
> 0 for nodes in such documents.  getComputedStyle throws.

Right, is that what we want to do? I guess if people really needed the style
information they should not set display:none on the iframe.

> I'm not sure what you mean by "under the sink"...

While the sink is loading stuff into the document and expects to be the one
calling InitialReflow on it. My point is that the sink is having enough trouble
with notifications and such as it is, without having to worry about that someone
might do initial reflow at any point in time.

> Back to this bug, the other option is to not construct the subdocument frame 
> for loading stuff.  Or something.  For objects with a standby attribute we
> want to show that instead, but almost no objects have one...

Hmm.. we'd need to show a partially loaded iframe though, no? Maybe we could
just avoid calling InitialReflow if the document is in the process of being
loaded, and rely on the sink to do it?
> While the sink is loading stuff into the document and expects to be the one
> calling InitialReflow on it.

Frankly, I think the fact that the sink is what calls InitialReflow is not a
very good setup.  There should be no real reason for the sink to even know that
presshells exist...

> Hmm.. we'd need to show a partially loaded iframe though, no?

Yeah.  I was sort of focusing on <object>/<embed> there...

> Maybe we could just avoid calling InitialReflow if the document is in the
> process of being loaded, and rely on the sink to do it?

We could, yes.  Again, that would mean that we'd try to paint the iframe when
there is nothing inside it.  I suppose we could handle that gracefully.

So sounds like we need an API on nsIDocument that indicates whether the document
is ready to be presented, and a way for the sink to affect this answer (either a
setter on nsIDocument or a getter on the sink).
Depends on: incrementalxml
Flags: blocking1.9a2?
Flags: blocking1.9a2? → blocking1.9-
Whiteboard: [wanted-1.9][reanalyze after XML incremental loading]
So is this better now that we have incremental XML?
Depends on: 370812
tor?  Is this still a problem?  The codepath I describe in comment 12 should work correctly now, I believe.
ping?  Is this still a problem?
The only assertion we get these days for this testcase is from session history ("###!!! ASSERTION: Adding child where we already have a child?  This will likely misbehave...").

Sounds like a worksforme...
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME
Flags: wanted1.9+
Whiteboard: [wanted-1.9][reanalyze after XML incremental loading] → [reanalyze after XML incremental loading]
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.