1472158 - No blobURL broadcasting

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

BlobURLs are currently broadcasted to any process. This is expensive from a memory point of view, and it requires several IPC calls each time a blobURL is created or revoked.

This bug is about removing the blobURL broadcasting.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - Retrieve the nsIPrincipal from BlobURLChannel

I'm going to ask for a review when the dependency bug is landed.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - No more BlobURLs sent to the content process.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - BlobURLs registered on the parent process

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - BlobURLInputStream for remote BlobURLs

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - Introduce BlobURLProtocolHandler::GetBlobURLPrincipalOrOwnerURI

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - Retrieve the principal from BlobURLChannel

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 3 - stop broadcatsing blobURLs

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 4 - Storing BlobURLs in the parent process

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 5 - Retrieve blobURL from the parent process

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 6 - tests

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 7 - documentation

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 4 - Storing BlobURLs in the parent process

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 5 - Retrieve blobURL from the parent process

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 5 - Retrieve blobURL from the parent process

Fully green on try.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9004519 [details] [diff] [review]
part 7 - documentation

>+/**
>+ * BlobURLs
>+ * ~~~~~~~~
>+ *
>+ * A blobURL is a URL connected to a DOM Blob content. It can be created and
>+ * used by any process. Each blobURL is registered in a hashtable in each
>+ * process,
really, each process? I thought the whole point was to avoid that.




>+ * BlobURLs and e10s
>+ * ~~~~~~~~~~~~~~~~~
>+ *
>+ * Another possible scenario is that, the current process wants to open an
>+ * unknown blobURL.
What is the other scenario? Something above? This isn't quite clear.




>+ * When a blobURL needs to be used, if it's unknown, a BlobURLChannel is
>+ * created, and this creates a BlobURLInputStream, which sends an IPC message to
>+ * the parent asking for that particular blobURL, as string
>+ * (ContentChild::RetrieveBlobURL)
>+ *
>+ * If the parent process known this blobURL, asynchornsly,
s/knows/

>+ it sends an IPC
s/asynchornsly/asynchronously/ but the sentence is a bit odd. asynchornsly seems to refer to knowning about the bloburl, not about
sending it. Please rephrase.



>+ * message to the content process, passing the BlobImpl and its nsIPrincipal
>+ * (ContentParent::SendBlobURLReady)
>+ *
>+ * When BlobURLInputStream receives these objects, it reads data from the
>+ * BlobImpl's nsIInputStream when needed. In the meantime, BlobURLInputStream
>+ * behaves as a nsIAsyncInputStream.
'In the meantime' seems to be useless part of the sentence. Or does BlobURLInputStream behave as
nsIAsyncInputStream only for some time?


>+ * When a blobURL, marked as revoked, needs to be opened, we return an error.
>+ * Otherwise, we continue as described previously.
I don't understand this explanation. So, revoked blobURL can't be opened, but what does 
"otherwise" refer to here?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9004518 [details] [diff] [review]
part 6 - tests

ok, this contains more than just tests.

We need to somehow ensure the blobs are in different processes. And need to have a test for revoking in one process and trying to use in another.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9004511 [details] [diff] [review]
part 1 - Introduce BlobURLProtocolHandler::GetBlobURLPrincipalOrOwnerURI

I don't understand the behavior of the new method. It returns blob url's principal or a null principal, but url is always just the blob url's principal's url.
and owner URI is returned even if blob url was revoked.


Rather odd inconsistency, and changes for example the behavior of NS_SecurityCompareURIs.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9004513 [details] [diff] [review]
part 2 - Retrieve the principal from BlobURLChannel

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  42eef1146aac56d92671881798f1062dff18b9f7
>
>diff --git a/caps/NullPrincipalURI.cpp b/caps/NullPrincipalURI.cpp
>--- a/caps/NullPrincipalURI.cpp
>+++ b/caps/NullPrincipalURI.cpp
>@@ -27,17 +27,16 @@ NullPrincipalURI::NullPrincipalURI()
> NullPrincipalURI::NullPrincipalURI(const NullPrincipalURI& aOther)
> {
>   mPath.Assign(aOther.mPath);
> }
> 
> nsresult
> NullPrincipalURI::Init()
> {
>-  // FIXME: bug 327161 -- make sure the uuid generator is reseeding-resistant.
>   nsCOMPtr<nsIUUIDGenerator> uuidgen = services::GetUUIDGenerator();
>   NS_ENSURE_TRUE(uuidgen, NS_ERROR_NOT_AVAILABLE);
> 
>   nsID id;
>   nsresult rv = uuidgen->GenerateUUIDInPlace(&id);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   mPath.SetLength(NSID_LENGTH - 1); // -1 because NSID_LENGTH counts the '\0'
>diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp
>--- a/caps/nsScriptSecurityManager.cpp
>+++ b/caps/nsScriptSecurityManager.cpp
>@@ -53,16 +53,17 @@
> #include "nsIURIFixup.h"
> #include "nsCDefaultURIFixup.h"
> #include "nsIChromeRegistry.h"
> #include "nsIResProtocolHandler.h"
> #include "nsIContentSecurityPolicy.h"
> #include "nsIAsyncVerifyRedirectCallback.h"
> #include "mozilla/Preferences.h"
> #include "mozilla/dom/BindingUtils.h"
>+#include "mozilla/dom/BlobURLChannel.h"
> #include "mozilla/NullPrincipal.h"
> #include <stdint.h>
> #include "mozilla/dom/ScriptSettings.h"
> #include "mozilla/ClearOnShutdown.h"
> #include "mozilla/StaticPtr.h"
> #include "nsContentUtils.h"
> #include "nsJSUtils.h"
> #include "nsILoadInfo.h"
>@@ -341,22 +342,22 @@ nsScriptSecurityManager::GetChannelResul
>   if (owner) {
>     CallQueryInterface(owner, aPrincipal);
>     if (*aPrincipal) {
>       return NS_OK;
>     }
>   }
> 
>   if (loadInfo) {
>-        if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
>-          MOZ_ALWAYS_TRUE(NS_SUCCEEDED(loadInfo->GetSandboxedLoadingPrincipal(aPrincipal)));
>-          MOZ_ASSERT(*aPrincipal);
>-          InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>-          return NS_OK;
>-        }
>+    if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
>+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(loadInfo->GetSandboxedLoadingPrincipal(aPrincipal)));
>+      MOZ_ASSERT(*aPrincipal);
>+      InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>+      return NS_OK;
>+    }
> 
>     bool forceInherit = loadInfo->GetForceInheritPrincipal();
>     if (aIgnoreSandboxing && !forceInherit) {
>       // Check if SEC_FORCE_INHERIT_PRINCIPAL was dropped because of
>       // sandboxing:
>       if (loadInfo->GetLoadingSandboxed() &&
>         loadInfo->GetForceInheritPrincipalDropped()) {
>         forceInherit = true;
>@@ -389,16 +390,26 @@ nsScriptSecurityManager::GetChannelResul
>                                                         uri,
>                                                         inheritForAboutBlank,
>                                                         false)) {
>         principalToInherit.forget(aPrincipal);
>         return NS_OK;
>       }
>     }
>   }
>+
>+  nsCOMPtr<nsIBlobURLChannel> blobURLChannel = do_QueryInterface(aChannel);
>+  if (blobURLChannel) {
>+    nsCOMPtr<nsIPrincipal> principal = blobURLChannel->GetPrincipal();
>+    if (principal) {
>+      principal.forget(aPrincipal);
>+      return NS_OK;
>+    }
>+  }
>+
>   nsresult rv = GetChannelURIPrincipal(aChannel, aPrincipal);
>   NS_ENSURE_SUCCESS(rv, rv);
>   InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>   return NS_OK;
> }
> 
> /* The principal of the URI that this channel is loading. This is never
>  * affected by things like sandboxed loads, or loads where we forcefully
>diff --git a/dom/file/uri/BlobURLChannel.cpp b/dom/file/uri/BlobURLChannel.cpp
>--- a/dom/file/uri/BlobURLChannel.cpp
>+++ b/dom/file/uri/BlobURLChannel.cpp
>@@ -4,16 +4,18 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "BlobURLChannel.h"
> #include "mozilla/dom/BlobImpl.h"
> 
> using namespace mozilla::dom;
> 
>+NS_IMPL_ISUPPORTS_INHERITED(BlobURLChannel, nsBaseChannel, nsIBlobURLChannel)
>+
> BlobURLChannel::BlobURLChannel(nsIURI* aURI,
>                                nsILoadInfo* aLoadInfo)
>   : mInitialized(false)
> {
>   SetURI(aURI);
>   SetOriginalURI(aURI);
>   SetLoadInfo(aLoadInfo);
> 
>@@ -30,17 +32,17 @@ void
> BlobURLChannel::InitFailed()
> {
>   MOZ_ASSERT(!mInitialized);
>   MOZ_ASSERT(!mInputStream);
>   mInitialized = true;
> }
> 
> void
>-BlobURLChannel::Initialize(BlobImpl* aBlobImpl)
>+BlobURLChannel::Initialize(BlobImpl* aBlobImpl, nsIPrincipal* aPrincipal)
> {
>   MOZ_ASSERT(!mInitialized);
> 
>   nsAutoString contentType;
>   aBlobImpl->GetType(contentType);
>   SetContentType(NS_ConvertUTF16toUTF8(contentType));
> 
>   if (aBlobImpl->IsFile()) {
>@@ -60,16 +62,18 @@ BlobURLChannel::Initialize(BlobImpl* aBl
> 
>   aBlobImpl->CreateInputStream(getter_AddRefs(mInputStream), rv);
>   if (NS_WARN_IF(rv.Failed())) {
>     InitFailed();
>     return;
>   }
> 
>   MOZ_ASSERT(mInputStream);
>+
>+  mPrincipal = aPrincipal;
>   mInitialized = true;
> }
> 
> nsresult
> BlobURLChannel::OpenContentStream(bool aAsync, nsIInputStream** aResult,
>                                   nsIChannel** aChannel)
> {
>   MOZ_ASSERT(mInitialized);
>@@ -85,9 +89,16 @@ BlobURLChannel::OpenContentStream(bool a
> 
>   return NS_OK;
> }
> 
> void
> BlobURLChannel::OnChannelDone()
> {
>   mInputStream = nullptr;
>+  mPrincipal = nullptr;
> }
>+
>+nsIPrincipal*
>+BlobURLChannel::GetPrincipal() const
>+{
>+  return mPrincipal;
>+}
>diff --git a/dom/file/uri/BlobURLChannel.h b/dom/file/uri/BlobURLChannel.h
>--- a/dom/file/uri/BlobURLChannel.h
>+++ b/dom/file/uri/BlobURLChannel.h
>@@ -13,40 +13,65 @@
> 
> class nsIURI;
> 
> namespace mozilla {
> namespace dom {
> 
> class BlobImpl;
> 
>-class BlobURLChannel final : public nsBaseChannel
>+#define BLOBURLCHANNEL_IID \
>+  { 0x128959c3, 0xf00d, 0x436b, \
>+    { 0xa2, 0xba, 0x4b, 0xd5, 0x45, 0x59, 0x04, 0x69 } }
>+
>+class nsIBlobURLChannel : public nsISupports
> {
> public:
>+  NS_DECLARE_STATIC_IID_ACCESSOR(BLOBURLCHANNEL_IID)
>+
>+  virtual nsIPrincipal*
>+  GetPrincipal() const = 0;
>+};
I don't see reason for nsIBlobURLChannel. BlobURLChannel could have IID itself, similar to dom::Element and such


> 
>+  // The principal of the Blob's parent.
parent? What is Blob's parent?

>+  nsCOMPtr<nsIPrincipal> principal;
>+
>+  // For revoked blobURLs, we use a null principal.
>+  if (blobURL->Revoked()) {
>+    principal = NullPrincipal::CreateWithoutOriginAttributes();
>+  }
>+
>+  // If it's a known blobURL, let's use the known principal.
>+  if (!principal) {
>+    DataInfo* info = GetDataInfoFromURI(aURI, true /*aAlsoIfRevoked */);
>+    if (info) {
>+      // We know that this is something else.
>+      if (info->mObjectType != DataInfo::eBlobImpl || !info->mBlobImpl) {
>+        return false;
>+      }
>+
>+      principal = info->mPrincipal;
>+    }
>+  }
>+
>+  if (principal) {
>+    if (aOwnerURI) {
>+      rv = principal->GetURI(aOwnerURI);
So here ownerURI can be also the null principal uri, but in the part 1 it wouldn't be.


>-  if (aPrincipal) {
>-    principal.forget(aPrincipal);
>+  // 'null/<uuid>' blobURLs should not leave the owning process.
>+  if (StringBeginsWith(path, NS_LITERAL_CSTRING("null/"))) {
>+    return false;
Why we do string comparison and not check whether it is a null principal?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9004690 [details] [diff] [review]
part 5 - Retrieve blobURL from the parent process

>+void
>+BlobURLChannel::InitializeRemote()
>+{
>+  MOZ_ASSERT(!XRE_IsParentProcess());
>+
>+  nsCOMPtr<nsIURI> uri;
>+  nsresult rv = GetURI(getter_AddRefs(uri));
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    InitFailed();
>+    return;
>+  }
>+
>+  RefPtr<BlobURL> blobURL;
>+  rv = uri->QueryInterface(kHOSTOBJECTURICID, getter_AddRefs(blobURL));
Huh, why is CID use for this. Could you file a followup to make BlobURL to have a normal IID

>+void
>+BlobURLChannel::SetPrincipal(nsIPrincipal* aPrincipal)
>+{
>+  MOZ_ASSERT(aPrincipal && !mPrincipal);
>+  MOZ_ASSERT_IF(mLoadInfo &&
>+                  !nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal()),
>+                 ChromeUtils::IsOriginAttributesEqualIgnoringFPD(mLoadInfo->GetOriginAttributes(),
>+                                                                 BasePrincipal::Cast(aPrincipal)->OriginAttributesRef()));
Why ignoring FPD?


>+void
>+BlobURLInputStream::DataReceived(BlobImpl* aBlobImpl,
>+                                 nsIPrincipal* aPrincipal)
DataReceived sounds a bit weird, when we receive BlobImpl.  And then looks like this can be called with null params.
Could this be something like... UpdateStreamContent? Not too good name either though.


>+  if (hasAsyncWaitCallback) {
>+    if (mInputStream) {
>+      mInputStream->AsyncWait(this, asyncWaitFlags, asyncWaitRequestedCount,
>+                              asyncWaitTarget);
>+    } else {
>+      RefPtr<BlobURLInputStream> self = this;
>+      nsCOMPtr<nsIRunnable> r =
>+        NS_NewRunnableFunction("BlobURLInputStream::DataReceived", [self]() {
>+          nsCOMPtr<nsIInputStreamCallback> callback;
>+          {
>+            MutexAutoLock lock(self->mMutex);
>+            callback = self->mAsyncWaitCallback;
>+          }
>+
>+          if (callback) {
>+            callback->OnInputStreamReady(self);
>+          }
>+        });
>+
>+      if (asyncWaitTarget) {
>+        asyncWaitTarget->Dispatch(r, NS_DISPATCH_NORMAL);
>+      } else {
>+        r->Run();
>+      }
>+    }
>+  }
>+}
This needs some comment what is happening. What is the runnable function doing and why?


>+class BlobURLInputStream final : public nsIAsyncInputStream
>+                               , public nsIInputStreamLength
>+                               , public nsIAsyncInputStreamLength
>+                               , public nsIInputStreamCallback
>+{
>+public:
>+  NS_DECL_THREADSAFE_ISUPPORTS
>+  NS_DECL_NSIINPUTSTREAM
>+  NS_DECL_NSIASYNCINPUTSTREAM
>+  NS_DECL_NSIINPUTSTREAMLENGTH
>+  NS_DECL_NSIASYNCINPUTSTREAMLENGTH
>+  NS_DECL_NSIINPUTSTREAMCALLBACK
>+
>+  static already_AddRefed<nsIInputStream>
>+  Create(BlobURLChannel* aChannel, BlobURL* aURI);
>+
>+  void
>+  DataReceived(BlobImpl* aBlobImpl,
>+               nsIPrincipal* aPrincipal);
>+
>+private:
>+  explicit BlobURLInputStream(BlobURLChannel* aChannel);
>+  ~BlobURLInputStream();
>+
>+  Mutex mMutex;
What all is protected by this mutex. Needs some documentation.

>+
>+  enum State {
{ goes to its own line.

>+    ePending,
>+    ePendingClosed,
>+    eReady,
>+    eError,
>+  };
These states need documentation.



>@@ -836,16 +844,19 @@ private:
>   nsCOMPtr<nsIFile> mProfileDir;
> #endif
> 
>   // Hashtable to keep track of the pending GetFilesHelper objects.
>   // This GetFilesHelperChild objects are removed when RecvGetFilesResponse is
>   // received.
>   nsRefPtrHashtable<nsIDHashKey, GetFilesHelperChild> mGetFilesPendingRequests;
> 
>+  // Hashtable to keep track of the pending BlobURLInputStream objects.
>+  nsRefPtrHashtable<nsIDHashKey, BlobURLInputStream> mBlobURLPendingStreams;
Perhaps mPendingBlobURLStreams would sound a tad better.




I'll need to read this several times. And we need quite a few tests for all this stuff.

Comment 22

[Tom Ritter [:tjr] (OOTO until mid-August)]

This is actually a fission blocker because it's about proactively broadcast data; rather than a rogue content process requesting it.

Comment 23

[Eric Rahm [:erahm]]

baku, are you planning on picking this back up? We're seeing crashes related to it over in bug 1513096 and it's definitely important for the content memshrink project.

Comment 24

[Andrea Marchesini [:baku]]

This issue will be resolved indirectly by fission project. We currently broadcast blobURLs, because, a process can load them at any time. When we load them, we need to extract the owning principal synchronously (See principal CheckMayLoad and subsumes methods).

But, after fission, we don't need to broadcast them anymore because only the owning origin is allowed to open its own blobURLs and, all of this will be running on the same process.

farre, Is this correct?

Comment 26

[Andreas Farre [:farre]]

That should be the case.

Comment 27

[Chris Peterson [:cpeterson]]

Baku, what is the current state of your BlobURL patches here?

We're tracking this bug as a potential blocker for Fission Nightly (M6).

Comment 28

[Andrea Marchesini [:baku]]

Attachment: Bug 1472158 - Broadcast BlobURLs only to processes with the same loaded origins, r=farre,asuth

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26694602791a
Broadcast BlobURLs only to processes with the same loaded origins, r=farre

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/26694602791a

Comment 32

[Andrei Ciure[:aciure]]

Backed out changeset 26694602791a (bug 1472158) for causing Bug 1583000

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=282265108&searchStr=windows%2C7%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-windows7-32%2Fdebug-web-platform-tests-e10s-5%2Cw%28wpt5%29&tochange=028b40d8140ccd71dde8bf1322df03247e64750c&fromchange=0cc5dce6603393d3e94a419861c03c44563a6bd7

backout: https://hg.mozilla.org/integration/autoland/rev/76a5cf41007c190e1c5ab515b4e274fb74043674

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout got merged: https://hg.mozilla.org/mozilla-central/rev/76a5cf41007c

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

The implementation that landed seems to only pay attention to window clients, and since ServiceWorkers are intentionally randomly allocated across all eligible processes, the test fails with high probability with a NetworkError when fetch fails to map the blob URL. (If ServiceWorkers used the same process-selection affinity as SharedWorkers, the test would have passed, but the patch would not handle SharedWorkers or ServiceWorkers correctly.)

The ClientManagerService already knows about all clients (which includes ServiceWorkers), and would be a better source of the information.

Note that I think there is also inherently a PContent/PBackground race in the broadcast mechanism given that the test already has some expected failures per https://searchfox.org/mozilla-central/source/testing/web-platform/meta/service-workers/service-worker/postmessage-blob-url.https.html.ini. (That is, the blob URL broadcast is sent over PContent, but the postMessage in the test goes via PBackground.)

Comment 35

[Andrew Sutherland [:asuth] (he/him)]

As a follow-up to my comment 34, baku and I discussed the options via slack and decided that expanding the existing patch approach to also be aware of RemoteWorkers was the most pragmatic solution. (Especially given that fission largely moots the entire problem space.)

The ClientManagerService and Clients API exist on PBackground and the IPC actors exist on a per-global basis, when the semantics we want are really per-process. That would suggest creating something like the oft-discussed background-service-in-the-child mechanism which the "DOM File" thread and RemoteWorkerService and its "Worker Launcher" thread approximate. However, even that complexity would still run into propagation races between PContent and PBackground (via multiple paths). Using the Clients API and making sure that the blobs are relayed to every top-level actor at least once (and therefore all PBackground connection pairs between content threads and PBackground in the parent) in addition to PContent would address the problem, but creates coordination problems from receiving N updates. Plus, it massively multiplies the impact of a badly behaving site.

Also clearing the needinfo for :baku since the problem is understood and addressed and a fixed patch is up.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9740bd7bf8f
Broadcast BlobURLs only to processes with the same loaded origins, r=farre,asuth

Comment 37

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b9740bd7bf8f

Comment 38

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b9740bd7bf8f

Comment 39

[Jens Stutte [:jstutte]]

Hi Adrian, as this bug is already resolved as FIXED, what does this dependency to bug 1636823 actually mean? (I interpret a dependency as "something that needs to be done before we can complete this work here" - which already has been completed in the past in this case.) Should we reopen this one? I'd prefer to just fix bug 1636823 as a normal defect of something we believed to work some time in the past.

If the intent is to say, the feature we implemented here once is now broken, I fear we have no consistent handling of "enhancement"s being "identifiable features" we could track defects against. I'd say, the most fine grained level we have for this in a quite consistent manner is the component itself (which we don't need to track as dependency, anyway) ?

Maybe a "see also" relation is more appropriate here to provide the wanted context? Am I missing something?

Comment 40

[Adrian Florinescu [:aflorinescu]]

So basically the block means that this fix introduced bug 1636823, which I'm uncertain if it is a regression or it is intended behavior. If it's a regression, then bug 1636823 will be treated separately from this one and fixed accordingly. Even If it's not a regression, but a follow-up, then it will also be treated separately.
Let's wait for :baku to weight in on bug 1636823 as this bug will remain fixed regardless of the resolution of bug 1636823.