Closed Bug 390385 Opened 17 years ago Closed 17 years ago

Plugins may get instantiated before first reflow

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: Biesinger, Assigned: Biesinger)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Attached patch patch (obsolete) (deleted) — Splinter Review
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?
Flags: blocking1.9?
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
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)
Attachment #274693 - Attachment is obsolete: true
Attachment #275146 - Flags: superreview?(bzbarsky)
Attachment #275146 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9 M8
So why is it OK to remove those Notify() calls? Are we sure that the state didn't change there or something?
what needs to be done will be done by the destructor of AutoNotifier.
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?
Attachment #275146 - Flags: superreview?(bzbarsky)
Attachment #275146 - Flags: superreview+
Attachment #275146 - Flags: review?(bzbarsky)
Attachment #275146 - Flags: review+
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 on attachment 275146 [details] [diff] [review] patch v2 this patch makes us more compatible with how 1.8 handled plugins
Attachment #275146 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Comment on attachment 275146 [details] [diff] [review] patch v2 clearing approval request as this is a blocker now
Attachment #275146 - Flags: approval1.9?
Attached patch merged to trunk (deleted) — Splinter Review
Attachment #275146 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: