Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Refactor nsObjectLoadingContent

RESOLVED FIXED in mozilla17

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: johns, Assigned: johns)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 31 obsolete attachments)

129.76 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
65.62 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
2.16 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 738396
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 728933
(Assignee)

Updated

5 years ago
Blocks: 752746
(Assignee)

Updated

5 years ago
Blocks: 96976
(Assignee)

Updated

5 years ago
Blocks: 157554
(Assignee)

Updated

5 years ago
Blocks: 195030
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
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
Attachment #623340 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
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
Attachment #623905 - Attachment is obsolete: true

Comment 4

5 years ago
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?
(Assignee)

Comment 5

5 years ago
Created attachment 632100 [details] [diff] [review]
[WIP] refactor loading path v0.4

https://tbpl.mozilla.org/?tree=Try&rev=c2f0680a84b3
Attachment #624226 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
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
Attachment #632100 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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
Attachment #632385 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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
Attachment #632947 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 633328 [details] [diff] [review]
[2/2] refactor fallback paths [WIP v0.7]

https://tbpl.mozilla.org/?tree=Try&rev=589590aed6be
Attachment #632949 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 765103

Comment 11

5 years ago
any idea if this will resolve bug 652300
(Assignee)

Comment 12

5 years ago
(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!
(Assignee)

Comment 13

5 years ago
Created attachment 634191 [details] [diff] [review]
[1/2] refactor loading path [WIP v0.8]

More test failure fixes
Attachment #633327 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 634193 [details] [diff] [review]
[2/2] refactor fallback paths [WIP v0.8]
Attachment #633328 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
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

Updated

5 years ago
Blocks: 652300
(Assignee)

Comment 16

5 years ago
Created attachment 635583 [details] [diff] [review]
[1/2] refactor loading path v1

https://tbpl.mozilla.org/?tree=Try&rev=931688144af2
Attachment #634191 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 635584 [details] [diff] [review]
[2/2] refactor fallback paths v1

https://tbpl.mozilla.org/?tree=Try&rev=bef48dfd34e4
Attachment #634193 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 635951 [details] [diff] [review]
[base] Backout Bug 406451 for regressions
(Assignee)

Comment 19

5 years ago
Created attachment 635952 [details] [diff] [review]
[1/1] Refactor nsObjectLoadingContent loading paths v2
Attachment #635583 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 635953 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2
Attachment #635584 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 635969 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2.1
Attachment #635953 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #635952 - Flags: superreview?(jst)
Attachment #635952 - Flags: review?(joshmoz)
(Assignee)

Comment 22

5 years ago
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
Attachment #635969 - Flags: superreview?(jst)
Attachment #635969 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Blocks: 767627
(Assignee)

Updated

5 years ago
Blocks: 767631
(Assignee)

Updated

5 years ago
Blocks: 767633
(Assignee)

Updated

5 years ago
Blocks: 767635
(Assignee)

Updated

5 years ago
Blocks: 767636
(Assignee)

Updated

5 years ago
Blocks: 693238
(Assignee)

Updated

5 years ago
Blocks: 548133
(Assignee)

Updated

5 years ago
Blocks: 767638
(Assignee)

Updated

5 years ago
Blocks: 767639
(Assignee)

Comment 23

5 years ago
Created attachment 637650 [details] [diff] [review]
[1/1] Refactor nsObjectLoadingContent loading paths v2.2
Attachment #635952 - Attachment is obsolete: true
Attachment #635952 - Flags: superreview?(jst)
Attachment #635952 - Flags: review?(joshmoz)
Attachment #637650 - Flags: superreview?(jst)
Attachment #637650 - Flags: review?(joshmoz)
(Assignee)

Comment 24

5 years ago
Created attachment 637651 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v2.2
Attachment #635969 - Attachment is obsolete: true
Attachment #635969 - Flags: superreview?(jst)
Attachment #635969 - Flags: review?(joshmoz)
Attachment #637651 - Flags: superreview?(jst)
Attachment #637651 - Flags: review?(joshmoz)
(Assignee)

Comment 25

5 years ago
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
Blocks: 751237

Comment 26

5 years ago
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//
Attachment #637650 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 27

5 years ago
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
Attachment #635951 - Attachment is obsolete: true
Attachment #637650 - Attachment is obsolete: true
Attachment #637652 - Attachment is obsolete: true
Attachment #637650 - Flags: superreview?(jst)
Attachment #639459 - Flags: superreview?(jst)
Attachment #639459 - Flags: review?(joshmoz)
(Assignee)

Comment 28

5 years ago
Created attachment 639460 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v3

Rebased
Attachment #637651 - Attachment is obsolete: true
Attachment #637651 - Flags: superreview?(jst)
Attachment #637651 - Flags: review?(joshmoz)
Attachment #639460 - Flags: superreview?(jst)
Attachment #639460 - Flags: review?(joshmoz)
(Assignee)

Comment 29

5 years ago
Created attachment 639474 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v3.1

Fix compile error from rebase...
Attachment #639459 - Attachment is obsolete: true
Attachment #639459 - Flags: superreview?(jst)
Attachment #639459 - Flags: review?(joshmoz)
Attachment #639474 - Flags: superreview?(jst)
Attachment #639474 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Depends on: 771666

Updated

5 years ago
Attachment #639474 - Flags: review?(joshmoz) → review+
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.
Attachment #639474 - Flags: superreview?(jst) → superreview+

Comment 31

5 years ago
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.
Attachment #639460 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 32

5 years ago
(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!
Attachment #639460 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 34

5 years ago
Created attachment 644118 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4, r=josh,sr=jst
Attachment #639474 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
Created attachment 644119 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4 r=josh,sr=jst
Attachment #639460 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Created attachment 644120 [details] [diff] [review]
Followup - consider capabilities when checking content policy
(Assignee)

Comment 37

5 years ago
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
Attachment #644120 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #644120 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 38

5 years ago
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

5 years ago
(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?
(Assignee)

Updated

5 years ago
Depends on: 777098
(Assignee)

Comment 40

5 years ago
Created attachment 645568 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst
Attachment #644118 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
Created attachment 645569 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst
Attachment #644119 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 645570 [details] [diff] [review]
Followup - consider capabilities when checking content policy v.1.1, r=josh
Attachment #644120 - Attachment is obsolete: true
(Assignee)

Comment 43

5 years ago
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
(Assignee)

Comment 44

5 years ago
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
(Assignee)

Comment 46

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 773207
(Assignee)

Comment 47

5 years ago
Created attachment 649378 [details] [diff] [review]
[1/2] Refactor nsObjectLoadingContent loading paths v4.1, r=josh,sr=jst
Attachment #645568 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 649379 [details] [diff] [review]
[2/2] Refactor nsObjectLoadingContent fallback paths v4.1, r=josh,sr=jst
Attachment #645569 - Attachment is obsolete: true
Attachment #645571 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
Created attachment 649381 [details] [diff] [review]
Followup - consider capabilities when checking content policy v.1.1, r=josh
Attachment #645570 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 52

5 years ago
Good work

Updated

5 years ago
Depends on: 780969
(Assignee)

Updated

5 years ago
Attachment #649378 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #649379 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #649381 - Flags: checkin+
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().
Depends on: 781126
(Assignee)

Updated

5 years ago
Blocks: 781310
Depends on: 781394

Updated

5 years ago
Blocks: 776208
Blocks: 762805

Updated

5 years ago
Depends on: 781272

Updated

5 years ago
No longer depends on: 781126

Updated

5 years ago
Depends on: 781126
Depends on: 782232

Updated

5 years ago
Depends on: 781265
(Assignee)

Updated

5 years ago
Depends on: 782384
(Assignee)

Updated

5 years ago
Depends on: 782703
(Assignee)

Updated

5 years ago
Depends on: 782707
(Assignee)

Comment 54

5 years ago
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
(Assignee)

Comment 55

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

Attached to wrong bug
Attachment #651880 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 783059
Depends on: 783351
(Assignee)

Updated

5 years ago
Blocks: 784185
(Assignee)

Updated

5 years ago
Blocks: 309531

Updated

5 years ago
Depends on: 786650
Depends on: 788031
Depends on: 788252

Updated

5 years ago
Depends on: 788902

Updated

5 years ago
Depends on: 789604

Updated

5 years ago
Depends on: 788107

Updated

5 years ago
Depends on: 791524

Updated

5 years ago
Depends on: 803159
(Assignee)

Updated

5 years ago
Depends on: 798026
Depends on: 787778
Depends on: 818802
(Assignee)

Updated

5 years ago
No longer blocks: 767627
(Assignee)

Updated

5 years ago
No longer blocks: 767633
No longer blocks: 728933

Updated

3 years ago
Depends on: 1065920
You need to log in before you can comment on or make changes to this bug.