745030 - Refactor nsObjectLoadingContent

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

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.

Comment 1

[John Schoenick [:johns]]

Attachment: [WIP] refactor loading path

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

Comment 2

[John Schoenick [:johns]]

Attachment: [WIP] refactor loading path

Another version which will hopefully burn down the tree less thoroughly

https://tbpl.mozilla.org/?tree=Try&rev=8637404386c2

Comment 3

[John Schoenick [:johns]]

Attachment: [WIP] refactor loading path v0.3

Another version that should fix remaining test failures.

https://tbpl.mozilla.org/?tree=Try&rev=00c255b7678f

Comment 4

[Danial Horton]

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?

Comment 5

[John Schoenick [:johns]]

Attachment: [WIP] refactor loading path v0.4

https://tbpl.mozilla.org/?tree=Try&rev=c2f0680a84b3

Comment 6

[John Schoenick [:johns]]

Attachment: [WIP] refactor loading path v0.5

Fixes the three test failures in previous version

https://tbpl.mozilla.org/?tree=Try&rev=568d5cf6e7ea

Comment 7

[John Schoenick [:johns]]

Attachment: [1/2] refactor loading path [WIP v0.6]

More fixes and a blind stab at fixing the intermittent plugin leak

https://tbpl.mozilla.org/?tree=Try&rev=57423f449433

Comment 8

[John Schoenick [:johns]]

Attachment: [2/2] refactor fallback paths [WIP v0.6]

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.

Comment 9

[John Schoenick [:johns]]

Attachment: [1/2] refactor loading path [WIP v0.7]

Try to stop the leak armageddon

https://tbpl.mozilla.org/?tree=Try&rev=212830dfd232

Comment 10

[John Schoenick [:johns]]

Attachment: [2/2] refactor fallback paths [WIP v0.7]

https://tbpl.mozilla.org/?tree=Try&rev=589590aed6be

Comment 11

[Danial Horton]

any idea if this will resolve bug 652300

Comment 12

[John Schoenick [:johns]]

(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!

Comment 13

[John Schoenick [:johns]]

Attachment: [1/2] refactor loading path [WIP v0.8]

More test failure fixes

Comment 14

[John Schoenick [:johns]]

Attachment: [2/2] refactor fallback paths [WIP v0.8]

Comment 15

[John Schoenick [:johns]]

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 16

[John Schoenick [:johns]]

Attachment: [1/2] refactor loading path v1

https://tbpl.mozilla.org/?tree=Try&rev=931688144af2

Comment 17

[John Schoenick [:johns]]

Attachment: [2/2] refactor fallback paths v1

https://tbpl.mozilla.org/?tree=Try&rev=bef48dfd34e4

Comment 18

[John Schoenick [:johns]]

Attachment: [base] Backout Bug 406451 for regressions

Comment 19

[John Schoenick [:johns]]

Attachment: [1/1] Refactor nsObjectLoadingContent loading paths v2

Comment 20

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v2

Comment 21

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v2.1

Comment 22

[John Schoenick [:johns]]

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

Comment 23

[John Schoenick [:johns]]

Attachment: [1/1] Refactor nsObjectLoadingContent loading paths v2.2

Comment 24

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v2.2

Comment 25

[John Schoenick [:johns]]

Attachment: Changes since last r? patch, for reference

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 26

[Josh Aas]

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//

Comment 27

[John Schoenick [:johns]]

Attachment: [1/1] Refactor nsObjectLoadingContent loading paths v3

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

Comment 28

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v3

Rebased

Comment 29

[John Schoenick [:johns]]

Attachment: [1/2] Refactor nsObjectLoadingContent loading paths v3.1

Fix compile error from rebase...

Comment 30

[Johnny Stenback (:jst)]

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 31

[Josh Aas]

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.

Comment 32

[John Schoenick [:johns]]

(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 33

[Johnny Stenback (:jst)]

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 34

[John Schoenick [:johns]]

Attachment: [1/2] Refactor nsObjectLoadingContent loading paths v4, r=josh,sr=jst

Comment 35

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v4 r=josh,sr=jst

Comment 36

[John Schoenick [:johns]]

Attachment: Followup - consider capabilities when checking content policy

Comment 37

[John Schoenick [:johns]]

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

Comment 38

[John Schoenick [:johns]]

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

Comment 39

[Josh Aas]

(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?

Comment 40

[John Schoenick [:johns]]

Attachment: [1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst

Comment 41

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst

Comment 42

[John Schoenick [:johns]]

Attachment: Followup - consider capabilities when checking content policy v.1.1, r=josh

Comment 43

[John Schoenick [:johns]]

Attachment: [merged into patch 1] baseURI is only a state change for java

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

Comment 44

[John Schoenick [:johns]]

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

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 46

[John Schoenick [:johns]]

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

Comment 47

[John Schoenick [:johns]]

Attachment: [1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst

Comment 48

[John Schoenick [:johns]]

Attachment: [2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst

Comment 49

[John Schoenick [:johns]]

Attachment: Followup - consider capabilities when checking content policy v.1.1, r=josh

Comment 50

[John Schoenick [:johns]]

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

Comment 51

[Ed Morley [:emorley]]

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

Comment 52

[Danial Horton]

Good work

Comment 53

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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().

Comment 54

[John Schoenick [:johns]]

Attachment: Change load blocker around in nsObjectLoadingContent (part of bug 767635 fixes)

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

Comment 55

[John Schoenick [:johns]]

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

Attached to wrong bug