390385 - Plugins may get instantiated before first reflow

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Plugins may get instantiated before the first reflow of the object frame.

This bug is for the case where a frame changes from display:none to inline (or whatever), i.e. the one where the frame calls HasNewFrame on the content.

That call should move to DidReflow with a check that it's the first reflow.

Regression from bug 1156

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

So basically this, however...

Consider this set of events:
- page has <object data="foo.gif" width="400" height="400"></object>
- javascript sets a classid attribute & changes the data attribute (the latter triggers LoadObject)
- LoadObject sees the classid attribute and flushes some stuff leading to frame creation but not to reflow
- It then calls Instantiate on the frame

- Later, reflow happens, that calls HasNewFrame on the content, that calls Instantiate again

that seems bad. Perhaps the GetFrame call that bug 381512 is adding to nsObjectLoadingContent::Instantiate should flush layout?

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

This patch does a few things:
- make sure that nsObjectLoadingContent::mContentType is correct (for the case where we instantiate by extension)
- remove some notify calls from nsObjectLoadingContent; instead, wait for the HasNewFrame call
- remove the FrameNeedsReflow calls from nsObjectFrame; the SetWindow call via Instantiate* should be enough. The widget showing is done by nsObjectFrame::CreateWidget.
- Fix the offset calculations in FixupWindow to match the ones done by DidReflow (note: This was inconsistent pre-bug 1156 too)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So why is it OK to remove those Notify() calls?  Are we sure that the state didn't change there or something?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

what needs to be done will be done by the destructor of AutoNotifier.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275146 [details] [diff] [review]
patch v2

Looks good.  It would also be nice to not flush reflow when we already have a reflowed frame in OnStartRequest...  can that happen, though?  If it can, file a followup to do that?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

you're right, I don't think we can have an object frame there, because we'd be in the loading state until then.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 275146 [details] [diff] [review]
patch v2

this patch makes us more compatible with how 1.8 handled plugins

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 275146 [details] [diff] [review]
patch v2

clearing approval request as this is a blocker now

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: merged to trunk

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.58; previous revision: 1.57
done
Checking in content/base/src/nsObjectLoadingContent.h;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.h,v  <--  nsObjectLoadingContent.h
new revision: 1.20; previous revision: 1.19
done
Checking in layout/generic/nsObjectFrame.cpp;
/cvsroot/mozilla/layout/generic/nsObjectFrame.cpp,v  <--  nsObjectFrame.cpp
new revision: 1.611; previous revision: 1.610
done

Comment 11

[Johnny Stenback (:jst)]

For the record, this caused bug 398213. We'll need to fix that regression, or we'll need to back this out, or find an alternate solution for the regression.