Last Comment Bug 745030 - Refactor nsObjectLoadingContent
: Refactor nsObjectLoadingContent
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: John Schoenick [:johns]
:
:
Mentors:
Depends on: 791524 1065920 771666 777098 780969 781126 781265 781272 781394 782232 782384 782703 782707 783351 786650 787778 788031 788107 788252 788902 789604 798026 803159 818802
Blocks: 751237 767631 767635 96976 157554 195030 309531 334288 418797 473344 548133 652300 682970 693238 736965 738396 741758 741778 744898 752746 762805 765103 767636 767638 767639 CVE-2012-1973 776208 781310 783059 784185
  Show dependency treegraph
 
Reported: 2012-04-12 17:06 PDT by John Schoenick [:johns]
Modified: 2014-09-11 06:07 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[WIP] refactor loading path (75.13 KB, patch)
2012-05-11 15:50 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP] refactor loading path (80.66 KB, patch)
2012-05-14 19:09 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP] refactor loading path v0.3 (81.43 KB, patch)
2012-05-15 16:03 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP] refactor loading path v0.4 (90.03 KB, patch)
2012-06-11 18:35 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP] refactor loading path v0.5 (91.34 KB, patch)
2012-06-12 13:31 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] refactor loading path [WIP v0.6] (92.74 KB, patch)
2012-06-13 16:28 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] refactor fallback paths [WIP v0.6] (63.45 KB, patch)
2012-06-13 16:30 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] refactor loading path [WIP v0.7] (97.16 KB, patch)
2012-06-14 17:19 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] refactor fallback paths [WIP v0.7] (63.33 KB, patch)
2012-06-14 17:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] refactor loading path [WIP v0.8] (97.30 KB, patch)
2012-06-18 15:06 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] refactor fallback paths [WIP v0.8] (66.44 KB, patch)
2012-06-18 15:06 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] refactor loading path v1 (116.68 KB, patch)
2012-06-21 19:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] refactor fallback paths v1 (46.48 KB, patch)
2012-06-21 19:40 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[base] Backout Bug 406451 for regressions (16.43 KB, patch)
2012-06-22 15:48 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/1] Refactor nsObjectLoadingContent loading paths v2 (128.92 KB, patch)
2012-06-22 15:49 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v2 (61.71 KB, patch)
2012-06-22 15:49 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v2.1 (59.83 KB, patch)
2012-06-22 16:33 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/1] Refactor nsObjectLoadingContent loading paths v2.2 (129.11 KB, patch)
2012-06-28 13:42 PDT, John Schoenick [:johns]
jaas: review-
Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v2.2 (60.00 KB, patch)
2012-06-28 13:42 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Changes since last r? patch, for reference (6.14 KB, patch)
2012-06-28 13:42 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/1] Refactor nsObjectLoadingContent loading paths v3 (128.57 KB, patch)
2012-07-05 13:53 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v3 (59.92 KB, patch)
2012-07-05 13:54 PDT, John Schoenick [:johns]
jaas: review+
jst: superreview+
Details | Diff | Splinter Review
[1/2] Refactor nsObjectLoadingContent loading paths v3.1 (129.06 KB, patch)
2012-07-05 14:32 PDT, John Schoenick [:johns]
jaas: review+
jst: superreview+
Details | Diff | Splinter Review
[1/2] Refactor nsObjectLoadingContent loading paths v4, r=josh,sr=jst (129.20 KB, patch)
2012-07-19 18:45 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v4 r=josh,sr=jst (63.78 KB, patch)
2012-07-19 18:46 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Followup - consider capabilities when checking content policy (2.08 KB, patch)
2012-07-19 18:46 PDT, John Schoenick [:johns]
jaas: review+
Details | Diff | Splinter Review
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst (129.65 KB, patch)
2012-07-24 16:17 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst (65.04 KB, patch)
2012-07-24 16:18 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Followup - consider capabilities when checking content policy v.1.1, r=josh (2.08 KB, patch)
2012-07-24 16:18 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[merged into patch 1] baseURI is only a state change for java (829 bytes, patch)
2012-07-24 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst (129.76 KB, patch)
2012-08-06 13:13 PDT, John Schoenick [:johns]
john: checkin+
Details | Diff | Splinter Review
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst (65.62 KB, patch)
2012-08-06 13:14 PDT, John Schoenick [:johns]
john: checkin+
Details | Diff | Splinter Review
Followup - consider capabilities when checking content policy v.1.1, r=josh (2.16 KB, patch)
2012-08-06 13:14 PDT, John Schoenick [:johns]
john: checkin+
Details | Diff | Splinter Review
Change load blocker around in nsObjectLoadingContent (part of bug 767635 fixes) (1.48 KB, patch)
2012-08-14 14:11 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review

Description John Schoenick [:johns] 2012-04-12 17:06:37 PDT
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] 2012-05-11 15:50:53 PDT
Created attachment 623340 [details] [diff] [review]
[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] 2012-05-14 19:09:26 PDT
Created attachment 623905 [details] [diff] [review]
[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] 2012-05-15 16:03:01 PDT
Created attachment 624226 [details] [diff] [review]
[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 2012-05-27 04:12:23 PDT
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] 2012-06-11 18:35:41 PDT
Created attachment 632100 [details] [diff] [review]
[WIP] refactor loading path v0.4

https://tbpl.mozilla.org/?tree=Try&rev=c2f0680a84b3
Comment 6 John Schoenick [:johns] 2012-06-12 13:31:00 PDT
Created attachment 632385 [details] [diff] [review]
[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] 2012-06-13 16:28:39 PDT
Created attachment 632947 [details] [diff] [review]
[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] 2012-06-13 16:30:48 PDT
Created attachment 632949 [details] [diff] [review]
[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] 2012-06-14 17:19:46 PDT
Created attachment 633327 [details] [diff] [review]
[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] 2012-06-14 17:20:26 PDT
Created attachment 633328 [details] [diff] [review]
[2/2] refactor fallback paths [WIP v0.7]

https://tbpl.mozilla.org/?tree=Try&rev=589590aed6be
Comment 11 Danial Horton 2012-06-18 09:24:40 PDT
any idea if this will resolve bug 652300
Comment 12 John Schoenick [:johns] 2012-06-18 15:04:35 PDT
(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] 2012-06-18 15:06:11 PDT
Created attachment 634191 [details] [diff] [review]
[1/2] refactor loading path [WIP v0.8]

More test failure fixes
Comment 14 John Schoenick [:johns] 2012-06-18 15:06:37 PDT
Created attachment 634193 [details] [diff] [review]
[2/2] refactor fallback paths [WIP v0.8]
Comment 15 John Schoenick [:johns] 2012-06-18 15:07:26 PDT
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] 2012-06-21 19:39:21 PDT
Created attachment 635583 [details] [diff] [review]
[1/2] refactor loading path v1

https://tbpl.mozilla.org/?tree=Try&rev=931688144af2
Comment 17 John Schoenick [:johns] 2012-06-21 19:40:01 PDT
Created attachment 635584 [details] [diff] [review]
[2/2] refactor fallback paths v1

https://tbpl.mozilla.org/?tree=Try&rev=bef48dfd34e4
Comment 18 John Schoenick [:johns] 2012-06-22 15:48:26 PDT
Created attachment 635951 [details] [diff] [review]
[base] Backout Bug 406451 for regressions
Comment 19 John Schoenick [:johns] 2012-06-22 15:49:05 PDT
Created attachment 635952 [details] [diff] [review]
[1/1] Refactor nsObjectLoadingContent loading paths v2
Comment 20 John Schoenick [:johns] 2012-06-22 15:49:25 PDT
Created attachment 635953 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2
Comment 21 John Schoenick [:johns] 2012-06-22 16:33:16 PDT
Created attachment 635969 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2.1
Comment 22 John Schoenick [:johns] 2012-06-22 16:50:43 PDT
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] 2012-06-28 13:42:49 PDT
Created attachment 637650 [details] [diff] [review]
[1/1] Refactor nsObjectLoadingContent loading paths v2.2
Comment 24 John Schoenick [:johns] 2012-06-28 13:42:53 PDT
Created attachment 637651 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2.2
Comment 25 John Schoenick [:johns] 2012-06-28 13:42:56 PDT
Created attachment 637652 [details] [diff] [review]
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 2012-07-05 13:19:59 PDT
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] 2012-07-05 13:53:38 PDT
Created attachment 639459 [details] [diff] [review]
[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] 2012-07-05 13:54:22 PDT
Created attachment 639460 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v3

Rebased
Comment 29 John Schoenick [:johns] 2012-07-05 14:32:29 PDT
Created attachment 639474 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v3.1

Fix compile error from rebase...
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-10 13:25:38 PDT
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 2012-07-12 07:09:56 PDT
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] 2012-07-12 10:33:39 PDT
(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, jst@mozilla.com) 2012-07-13 14:15:43 PDT
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] 2012-07-19 18:45:51 PDT
Created attachment 644118 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4, r=josh,sr=jst
Comment 35 John Schoenick [:johns] 2012-07-19 18:46:18 PDT
Created attachment 644119 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4 r=josh,sr=jst
Comment 36 John Schoenick [:johns] 2012-07-19 18:46:47 PDT
Created attachment 644120 [details] [diff] [review]
Followup - consider capabilities when checking content policy
Comment 37 John Schoenick [:johns] 2012-07-19 18:47:56 PDT
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] 2012-07-20 13:34:28 PDT
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 2012-07-24 07:58:12 PDT
(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] 2012-07-24 16:17:32 PDT
Created attachment 645568 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst
Comment 41 John Schoenick [:johns] 2012-07-24 16:18:04 PDT
Created attachment 645569 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst
Comment 42 John Schoenick [:johns] 2012-07-24 16:18:34 PDT
Created attachment 645570 [details] [diff] [review]
Followup - consider capabilities when checking content policy v.1.1, r=josh
Comment 43 John Schoenick [:johns] 2012-07-24 16:20:59 PDT
Created attachment 645571 [details] [diff] [review]
[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 46 John Schoenick [:johns] 2012-07-25 13:45:28 PDT
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] 2012-08-06 13:13:55 PDT
Created attachment 649378 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst
Comment 48 John Schoenick [:johns] 2012-08-06 13:14:22 PDT
Created attachment 649379 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst
Comment 49 John Schoenick [:johns] 2012-08-06 13:14:55 PDT
Created attachment 649381 [details] [diff] [review]
Followup - consider capabilities when checking content policy v.1.1, r=josh
Comment 52 Danial Horton 2012-08-07 07:39:49 PDT
Good work
Comment 53 :Ms2ger (⌚ UTC+1/+2) 2012-08-08 01:28:54 PDT
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] 2012-08-14 14:11:48 PDT
Created attachment 651880 [details] [diff] [review]
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] 2012-08-14 14:13:50 PDT
Comment on attachment 651880 [details] [diff] [review]
Change load blocker around in nsObjectLoadingContent (part of bug 767635 fixes)

Attached to wrong bug

Note You need to log in before you can comment on or make changes to this bug.