Meta bug - Refactoring the loading path of nsObjectLoadingContent to fix regressions from Bug 406541 as well as various plugin-loading/frame issues I found reviewing this code.

This breaks more than it fixes right now, but compiles and works on youtube, yay. Pushing to try just to see what we'll potentially be burning down, though we have an extreme lack of plugin tests https://tbpl.mozilla.org/?tree=Try&rev=bc77842b3daa

Another version which will hopefully burn down the tree less thoroughly https://tbpl.mozilla.org/?tree=Try&rev=8637404386c2

Another version that should fix remaining test failures. https://tbpl.mozilla.org/?tree=Try&rev=00c255b7678f

This refactoring will also resolve the cases of Flash content continuing to play after the parent page has been navigated away from (youtube and dailymotion) i assume?

Fixes the three test failures in previous version https://tbpl.mozilla.org/?tree=Try&rev=568d5cf6e7ea

More fixes and a blind stab at fixing the intermittent plugin leak https://tbpl.mozilla.org/?tree=Try&rev=57423f449433

https://tbpl.mozilla.org/?tree=Try&rev=59d72d5cbfe6 Refactor all the fallback related code to be much more sane. Does away with AutoNotifier/AutoFallback, introduces LoadFallback(). More explicitly handle fallback loading in LoadObject() rather than having stack classes catch error conditions and the like.

Try to stop the leak armageddon https://tbpl.mozilla.org/?tree=Try&rev=212830dfd232

any idea if this will resolve bug 652300

(In reply to Danial Horton from comment #11) > any idea if this will resolve bug 652300 I've no idea what might be going on there, however, I did fix a few race conditions related to plugin instantiation - so maybe!

More test failure fixes

Try pushes for v0.8: Refactor: https://tbpl.mozilla.org/?tree=Try&rev=4a046500f35c Refactor + Fallback cleanup: https://tbpl.mozilla.org/?tree=Try&rev=d6c7bf9396d4

Comment on attachment 635969 [details] [diff] [review] [2/2] Refactor nsObjectLoadingContent fallback paths v2.1 Ready for review, I think. Note that these both apply on top of the backout pending in bug 406541, see comments there. I included the backout patch here for simplicity. Some notes on approach: The first patch focuses on cleaning up the LoadObject path and fixing numerous oddities in the class. It adds an "UpdateObjectParameters" method that introspects* the object and determines what type to load, and whether or not to open a channel. LoadObject then does additional security checks and either loads that type, or a fallback type. If a channel is opened, mType is left as 'Loading' until mChannelLoaded is set by OnStartRequest, which then re-calls LoadObject to finish the process. This is a huge improvement over the spaghetti we had mixed between LoadObject and OnStartRequest previously, and ensures numerous security checks are not skipped due to odd load paths. LoadObject() also guarantees the 'mType' field be consistent, so checks like InitializePluginInstance() can be sure a plugin is actually security/sanity-checked and ready to load. The second patch mostly focuses on getting rid of AutoNotifier and AutoFallback and the weird initialization of fallback through Fallback(), UpdateFallbackState(), and check-stack-variables-on-return rules that made things more complicated. Since everything is handled in LoadObject() now, it is much cleaner to just explicitly call LoadFallback() and NotifyStateChange() as necessary. These patches don't address a few things that I'm opening followup bugs for, notably: - The InitializePluginInstance/AsyncStartPluginInstance/StartPluginInstance calls could use some cleanup, including figuring out why we're piling start-plugin runnables onto the stack (new checks in mType means we wont race-load invalid types, however) - Similarly, StopPluginInstance/DoStopPlugin/DoDelayedStop are a bit verbose and potentially racey with some start-stop conditions - pluginHost now has unreachable code and duplicated logic that can now all be cleaned up (there were still edge cases related to opening channels that could trigger this zombie-version of the code) * Ideally nsHTML{Shared,}ObjectElement would provide ::SetURI ::SetCodebase ::SetType overloads rather than just calling LoadObject and objlc looking at element properties, but this adds a lot of unnecessary complexity Try pushes: https://tbpl.mozilla.org/?tree=Try&rev=7dab823e2c9a https://tbpl.mozilla.org/?tree=Try&rev=c8ba6c502c2a

Minor update to the patches to fix a few late-changes that caused try failures. If you've started looking at the previous versions, I've attached a crossdiff of what changed -- I moved an assertion down, and changed the way UnloadContent() works a bit. These versions are back to all green on try: https://tbpl.mozilla.org/?tree=Try&rev=9d690fb3009a https://tbpl.mozilla.org/?tree=Try&rev=eaa12bff98c7

Comment on attachment 637650 [details] [diff] [review] [1/1] Refactor nsObjectLoadingContent loading paths v2.2 Review of attachment 637650 [details] [diff] [review]: ----------------------------------------------------------------- This is a huge effort, thanks! I'm going to avoid putting too many nits into my review, minimize comments so you can land soon and avoid dragging this out and avoid bitrot. r- mostly just for the uninitialized contentPolicy variable. If you can explain why that isn't a problem I'll change to r+. ::: content/base/src/nsObjectLoadingContent.cpp @@ +456,5 @@ > PluginSupportState mPluginState; > }; > > +// XXX You can't take the address of bitfield members, so we have two separate > +// classes for these :-/ XXX comments imply a problem or a TODO (to me, at least). If this isn't the case--there isn't a bug or a better way to write the code--then please remove the XXX. @@ +618,5 @@ > + NS_NOTREACHED("No pluginhost"); > + return false; > + } > + > + nsresult rv = pluginHost->IsPluginEnabledForType(aMIMEType.get()); This method name is a yes or no question, and the result is being treated as such, would be nice if it returned a bool. I think this might have been changed in another patch I reviewed in the past week from David Keeler. @@ +657,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + if (permission == nsIPermissionManager::ALLOW_ACTION) { > + mShouldPlay = true; > + } else { > + return NS_ERROR_PLUGIN_CLICKTOPLAY; This patch may need to be reconciled with some recent work from David Keeler. @@ +1695,5 @@ > + // with the channel if it could be any acceptable type. This type is > + // passed to OpenChannel() as the LoadType. We pass through LoadObject > + // again once the channel is opened and we're actually loading, so if > + // the final URI doesn't pass the now-known type, we'll abort. > + PRInt16 contentPolicy; Seems like there is the potential for this to get used uninitialized by the following code. @@ +1698,5 @@ > + // the final URI doesn't pass the now-known type, we'll abort. > + PRInt16 contentPolicy; > + bool allowLoad = false; > + PRInt32 policyType; > + if (!allowLoad && (mType == eType_Image || mType == eType_Loading)) { 'allowLoad' is always false here, this check is unnecessary. ::: content/base/src/nsObjectLoadingContent.h @@ +65,5 @@ > + // Content is a *non-svg* image > + eType_Image = TYPE_IMAGE, > + // Content is a plugin > + eType_Plugin = TYPE_PLUGIN, > + // Content is a subdocument, possibly an SVG Typo in comment, s/an//

Rebased on central (the backout patch landed and is no longer needed) and updated with Josh's comments

Rebased

Fix compile error from rebase...

Comment on attachment 639474 [details] [diff] [review] [1/2] Refactor nsObjectLoadingContent loading paths v3.1 This looks really good! Couple of small things below, mostly just style nits that caught my eye as I was reading through this. class InDocCheckEvent : public nsRunnable { public: - nsCOMPtr<nsIContent> mContent; - - InDocCheckEvent(nsIContent* aContent) + InDocCheckEvent(nsObjectLoadingContent* aContent) : mContent(aContent) { + static_cast<nsIObjectLoadingContent *>(mContent)->AddRef(); } ~InDocCheckEvent() { + static_cast<nsIObjectLoadingContent *>(mContent)->Release(); } + NS_IMETHOD Run(); - NS_IMETHOD Run(); +private: + nsObjectLoadingContent *mContent; Any reason *not* to make mContent an nsRefPtr<nsObjectLoadingContent>? That way you wouldn't need to manually call addref and release in this class. NS_IMETHODIMP nsPluginErrorEvent::Run() { - LOG(("OBJLC []: Firing plugin not found event for content %p\n", + LOG(("OBJLC [static]: Firing plugin not found event for content %p\n", mContent.get())); What does [static] mean here (and elsewhere)? - In CanHandleURI(nsIURI* aURI): + nsCOMPtr<nsIExternalProtocolHandler> extHandler = + do_QueryInterface(handler); Nit: indent the second line there... - In nsObjectLoadingContent::IsPluginEnabledByExtension(): + nsRefPtr<nsPluginHost> pluginHost = + already_AddRefed<nsPluginHost>(nsPluginHost::GetInst()); Please fix nsPluginHost::GetInst() to return already_AddRefed<nsPluginHost> so it's less easy to shoot oneself in ones own foot while you're at it. And then remove all the casts you had to add here. Or do this as a followup bug, that works too. - In nsObjectLoadingContent::IsPluginEnabledForType(): + nsRefPtr<nsPluginHost> pluginHost = + already_AddRefed<nsPluginHost>(nsPluginHost::GetInst()); Nit: indent second line... - In nsObjectLoadingContent::IsSupportedDocument(): + nsCOMPtr<nsIContent> thisContent = + do_QueryInterface(static_cast<nsIImageLoadingContent*>(this)); Nit: indent second line... + nsCOMPtr<nsIStreamConverterService> convServ = + do_GetService("@mozilla.org/streamConverters;1"); Nit: indent second line... - In nsObjectLoadingContent::InstantiatePluginInstance(): + nsRefPtr<nsPluginHost> pluginHost = + already_AddRefed<nsPluginHost>(nsPluginHost::GetInst()); Nit: indent second line... nsCOMPtr<nsIBlocklistService> blocklist = do_GetService("@mozilla.org/extensions/blocklist;1"); Not your change, but wanna fix the indentation here as well? +nsObjectLoadingContent::CloseChannel() { Nit: opening brace on it's own line. r=jst with those fixed.

Comment on attachment 639460 [details] [diff] [review] [2/2] Refactor nsObjectLoadingContent fallback paths v3 Review of attachment 639460 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsObjectLoadingContent.cpp @@ +1605,2 @@ > mType = eType_Null; > + fallbackType = eFallbackClickToPlay; Will doing this after potentially setting fallbackType to user disabled or suppressed mean that we might present click-to-play as overriding those states? Seems like disabled or suppressed should override click-to-play, not the other way around.

(In reply to Josh Aas (Mozilla Corporation) from comment #31) > Will doing this after potentially setting fallbackType to user disabled or > suppressed mean that we might present click-to-play as overriding those > states? Seems like disabled or suppressed should override click-to-play, not > the other way around. This should be okay - we only set eFallbackClickToPlay if mType is plugin at this point, whereas anything that switches the object to fallback mode would have already set type to null

Comment on attachment 639460 [details] [diff] [review] [2/2] Refactor nsObjectLoadingContent fallback paths v3 This looks great! Only spotted a couple of tiny nits while reading through this patch: +nsObjectLoadingContent::LoadFallback(FallbackType aType, bool aNotify) { Opening brace on its own line. + nsCOMPtr<nsIContent> thisContent = + do_QueryInterface(static_cast<nsIImageLoadingContent*>(this)); Indent second line there. sr=jst Seems we're at a good point to land this early next week!

Comment on attachment 644120 [details] [diff] [review] Followup - consider capabilities when checking content policy Something I noticed while reading over this again - we should only check content policy for some types if we support those types in the first place, or we might load superfluous channels

Arrgh, these no longer pass try after rebasing: https://tbpl.mozilla.org/?tree=Try&rev=8fd7074b7b23 The crash is in javascript proto unwrapping, likely due to bug 771202. Investigating

(In reply to John Schoenick [:johns] from comment #38) > The crash is in javascript proto unwrapping, likely due to bug 771202. > Investigating Any progress on this?

This change in patch 1/2 fixes the test failures from bug 771202

https://hg.mozilla.org/integration/mozilla-inbound/rev/83bbed851a1b https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d852465790 https://hg.mozilla.org/integration/mozilla-inbound/rev/11b6b9d60806

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5231ba7849 https://hg.mozilla.org/integration/mozilla-inbound/rev/aef63a0ce60c

Backed out due to random oranges: https://tbpl.mozilla.org/php/getParsedLog.php?id=13845528&tree=Mozilla-Inbound&full=1 This is probably exposing more issues in Bug 771202 similar to bug 777098, looking into it

Bug 771666 landed, let's try again https://hg.mozilla.org/integration/mozilla-inbound/rev/40a8737b62a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/615a657305b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bd764deb31

https://hg.mozilla.org/mozilla-central/rev/40a8737b62a1 https://hg.mozilla.org/mozilla-central/rev/615a657305b5 https://hg.mozilla.org/mozilla-central/rev/f3bd764deb31

Good work

Please file a followup to make nsPluginHost::GetInst() return already_AddRefed, and to check if <http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#4159> leaks; and one to replace the Assign("") calls with Truncate().

This hack should fix the behavior if that is what's happening here, but 767635 should fix the start/stop logic more thoroughly

Comment on attachment 651880 [details] [diff] [review] Change load blocker around in nsObjectLoadingContent (part of bug 767635 fixes) Attached to wrong bug