1354443 - Investigate the lifetime of nsPrintEngine, nsPrintData and nsPrintObject if they're really safe

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that managing nsPrintData instances and nsPrintObject instances is really terrible.

They are destroyed with UniquePtr, however, most referrers refer the instances with raw pointers which may be members of different life time objects (including refcountable object, like nsPagePrintTimer). If we could always use UniquePtr<T>& instead, may be safer than now.  However, I don't believe that it'll be kept safe forever because such rule may be ignored easily.

Additionally, nsPrintEngine uses PresShell::FlushPendingNotifications(). So, it's too difficult to understand what may occur during reflow.

Perhaps, related to bug 1342994.

Comment 1

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-high because it sounds like there are almost certainly some outstanding issues this will fix.

Comment 2

[Daniel Holbert [:dholbert]]

Note that the UniquePtr usage here (from bug bug 1342994) was just a direct conversion from preexisting raw pointers + manual "new"/"delete" invocations.  That bug upgraded implicit/hand-wavy ownership & sharing of these objects into something more explicit & grokkable, without modifying behavior. (That was the intent at least.)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Taking.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Make nsPrintData refcountable

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Make nsPrintObject refcountable

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Get rid of nsPrintData::mSelectedPO since nobody refers it

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Rewrite for loops referring nsPrintData::mPrintDocList

Rewriting for loops which refers nsPrintData::mPrintDocList with range based for loop.

Additionally, before referring all of mPrintDocList, it should grab nsPrintData instance.  That guarantees all instances referred by mPrintDocList.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Grab nsPrintObject before referring its kids with for loop

Grabbing nsPrintObject with RefPtr means that it guarantees all items of nsPrintObject::mKids won't be deleted.  So, before referring them with for loop, it should be grabbed.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Uplifting them to Aurora or older branches needs to rewrite the patches due to bug 1342994.

I already have the patches for the other branches, I think that only part.2 needs additional review again because it needs to include some code added by bug 1342994.

Patches for Aurora can be applied to Beta and ESR52. However, only part.1 for ESR52 needs to merge due to non-related part difference.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8860348 [details] [diff] [review]
part.1 Make nsPrintData refcountable

Review of attachment 8860348 [details] [diff] [review]:
-----------------------------------------------------------------

The commit messages all need the bug number substituted in for "xxxxxx", of course, but I suspect you already know that. :)

r=me with the following:

::: layout/printing/nsPrintData.h
@@ +3,5 @@
>   * 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/. */
>  
> +#ifndef nsPrintData_h
> +#define nsPrintData_h

Is there a reason you're removing the trailing underscores from the include guard here?  (and in just this .h file and not the other?)

As I recall, the trailing-underscore style is the prevailing (and hence "correct"-ish) style for pre-"namespace mozilla" headers.  So I think we want to keep these underscores.

(But even if it was correct to remove them, let's do that separately from this bug since it's not related to what you're changing here, and there are probably many more files that we'd want to fix up at the same time for consistency.)

@@ +98,3 @@
>  };
>  
> +#endif // #ifndef nsPrintData_h

Please revert this #endif change, too.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8860349 [details] [diff] [review]
part.2 Make nsPrintObject refcountable

Review of attachment 8860349 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/printing/nsPrintEngine.cpp
@@ +3105,1 @@
>  nsPrintEngine::FindPrintObjectByDOMWin(nsPrintObject* aPO,

Should this take a "const RefPtr<nsPrintObject>&" like all the other functions here (instead of a raw pointer)?

Or alternately: maybe this function should just return a raw pointer, and let the caller decide whether it wants to place that into a RefPtr or not?

Right now, it's a bit asymmetrical & nonsensical-seeming that this function *takes* a raw pointer (trusting that the caller keeps the args alive) but *returns* a refcounted pointer to something owned by that raw pointer.  It makes it seem like we're worried that the thing we're returning might be destroyed if we didn't return it as a refcounted pointer -- even though it's actually owned by something that we're already trusting the caller to keep alive for us.

@@ +3114,5 @@
>    }
>  
>    nsCOMPtr<nsIDocument> doc = aDOMWin->GetDoc();
>    if (aPO->mDocument && aPO->mDocument->GetOriginalDocument() == doc) {
> +    return RefPtr<nsPrintObject>(aPO).forget();

Please replace with:
  "return do_AddRef(aPO);"

(unless you simplify this function to return a raw pointer per my comment above -- in which case then this line would just want to go back to "return aPO;")

@@ +3810,5 @@
>    PR_PL(("Doc List\n***************************************************\n"));
>    PR_PL(("T  P A H    PO    DocShell   Seq     Page      Root     Page#    Rect\n"));
>    int32_t cnt = aDocList->Length();
>    for (int32_t i=0;i<cnt;i++) {
> +    RefPtr<nsPrintObject> po = aDocList->ElementAt(i);

No need to refcount here (or make any changes to this function at all), I think. This is in some not-super-important #ifdeffed-off-by-default debugging code, and it's just dumping the contents of an object that it's trusting the caller to keep alive.  So it seems to me that it should trust that the object's entries aren't going to spontaneously die mid-function.

(Unless I'm missing something)

@@ +3846,5 @@
>    }
>    int32_t cnt = aPO->mKids.Length();
>    for (int32_t i=0;i<cnt;i++) {
> +    const RefPtr<nsPrintObject>& po = aPO->mKids.ElementAt(i);
> +    MOZ_ASSERT(po);

As above, I don't think we need to bother messing with this dump function.  Can't we just keep this as "nsPrintObject* po = [...]"?  (and rely on the RefPtr-to-raw-pointer magic autocasting)

@@ +3907,5 @@
>  
>      int32_t cnt = aPO->mKids.Length();
>      for (int32_t i=0;i<cnt;i++) {
> +      const RefPtr<nsPrintObject>& po = aPO->mKids.ElementAt(i);
> +      MOZ_ASSERT(po);

Same here. Can't "po" just stay a raw pointer in this dump function?

::: layout/printing/nsPrintObject.h
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
> +#ifndef nsPrintObject_h
> +#define nsPrintObject_h

As in part 1, let's not clutter up this bug's patches with #include guard adjustments.

Comment 12

[Daniel Holbert [:dholbert]]

Before I proceed further with review here (and before you bother addressing my review comments so far), let's step back and make sure this change actually makes sense.

Right now the intent is clearly that nsPrintData and nsPrintObject instances are singly-owned, and they all chain up to a nsPrintEngine instance as their owner, I think.

If we make them refcounted, their ownership/lifetime get safer perhaps, but it's a bit of a sledgehammer, and their intended lifetimes/ownership get much fuzzier & harder to reason about.  (And there's some perf impact from refcount churn, which probably isn't a huge deal here but be nice to avoid if we don't actually need it.)

Let's look at the concerns you mentioned in comment 0:

(In reply to Masayuki Nakano [:masayuki] from comment #0)
> They are destroyed with UniquePtr, however, most referrers refer the
> instances with raw pointers which may be members of different life time
> objects (including refcountable object, like nsPagePrintTimer).

Note that nsPagePrintTimer does have a "Disconnect" method which gets called from nsPrintEngine.cpp ("DisconnectPagePrintTimer()"), which nulls out one of its pointers already (for safety).

Perhaps we should broaden that Disconnect() method to also null out nsPagePrintTimer::mPrintObj, too?  Would that clear this pointer in time?

> Additionally, nsPrintEngine uses PresShell::FlushPendingNotifications().

If you're worried about things being destroyed during its call to FlushPendingNotifications (a valid concern!), then you should probably be worried about nsPrintEngine *itself* being destroyed, since it's the ultimate owner of the nsPrintObject and nsPrintData instances.  And superficially, it looks like it might be possible for nsPrintEngine to be destroyed during the FlushPendingNotifications call -- there's no deathgrip keeping it alive at least. So perhaps we should add a deathgrip RefPtr wrapping the nsPrintEngine, at some level of the callstack into FlushPendingNotifications?

Or alternately, if you're worried about us reentering this code inside of FlushPendingNotifications call (not sure if that's possible), then perhaps we should add a boolean member-variable to guard against reentrancy?

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8860350 [details] [diff] [review]
part.3 Get rid of nsPrintData::mSelectedPO since nobody refers it

Review of attachment 8860350 [details] [diff] [review]:
-----------------------------------------------------------------

(Well, I guess I can r+ part 3 since it's just dead-code-removal & is clearly-OK regardless of what strategy we go with for the rest of this bug. :)  It might be better to land this part on its independently on its own bug, since it's independent of the rest of the changes here, though.)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Comment on attachment 8860349 [details] [diff] [review]
> part.2 Make nsPrintObject refcountable
> 
> Review of attachment 8860349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/printing/nsPrintEngine.cpp
> @@ +3105,1 @@
> >  nsPrintEngine::FindPrintObjectByDOMWin(nsPrintObject* aPO,
> 
> Should this take a "const RefPtr<nsPrintObject>&" like all the other
> functions here (instead of a raw pointer)?

Although, it's changed in part.5, but I move it to this.

> Or alternately: maybe this function should just return a raw pointer, and
> let the caller decide whether it wants to place that into a RefPtr or not?

I think that returning strong pointer is safer for security and stability. If that causes memory leak, it should be detected at running the automated tests. But anyway, it should be managed carefully if it's grabbed long time as member of a class.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8860348 [details] [diff] [review]
> part.1 Make nsPrintData refcountable
> 
> Review of attachment 8860348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The commit messages all need the bug number substituted in for "xxxxxx", of
> course, but I suspect you already know that. :)
> 
> r=me with the following:
> 
> ::: layout/printing/nsPrintData.h
> @@ +3,5 @@
> >   * 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/. */
> >  
> > +#ifndef nsPrintData_h
> > +#define nsPrintData_h
> 
> Is there a reason you're removing the trailing underscores from the include
> guard here?  (and in just this .h file and not the other?)

IIRC, we shouldn't use two or more underscores.

Additionally, our coding rule defines now here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

But I'll remove the changes.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Comment on attachment 8860350 [details] [diff] [review]
> part.3 Get rid of nsPrintData::mSelectedPO since nobody refers it
> 
> Review of attachment 8860350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Well, I guess I can r+ part 3 since it's just dead-code-removal & is
> clearly-OK regardless of what strategy we go with for the rest of this bug.

Because I wanted the guarantee that it's not actually unused and it's not necessary to be a strong pointer.

> It might be better to land this part on its independently on its own
> bug, since it's independent of the rest of the changes here, though.)

Okay.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> (In reply to Masayuki Nakano [:masayuki] from comment #0)
> > They are destroyed with UniquePtr, however, most referrers refer the
> > instances with raw pointers which may be members of different life time
> > objects (including refcountable object, like nsPagePrintTimer).
> 
> Note that nsPagePrintTimer does have a "Disconnect" method which gets called
> from nsPrintEngine.cpp ("DisconnectPagePrintTimer()"), which nulls out one
> of its pointers already (for safety).
> 
> Perhaps we should broaden that Disconnect() method to also null out
> nsPagePrintTimer::mPrintObj, too?  Would that clear this pointer in time?

Oh, I didn't realize that and yes, it should be cleared in Disconnect() but enough safe.

> > Additionally, nsPrintEngine uses PresShell::FlushPendingNotifications().
> 
> If you're worried about things being destroyed during its call to
> FlushPendingNotifications (a valid concern!), then you should probably be
> worried about nsPrintEngine *itself* being destroyed, since it's the
> ultimate owner of the nsPrintObject and nsPrintData instances.  And
> superficially, it looks like it might be possible for nsPrintEngine to be
> destroyed during the FlushPendingNotifications call -- there's no deathgrip
> keeping it alive at least. So perhaps we should add a deathgrip RefPtr
> wrapping the nsPrintEngine, at some level of the callstack into
> FlushPendingNotifications?

Sure, makes sense.

> Or alternately, if you're worried about us reentering this code inside of
> FlushPendingNotifications call (not sure if that's possible), then perhaps
> we should add a boolean member-variable to guard against reentrancy?

I don't worry about that since IIRC, FlushPendingNotifications() doesn't allow to run itself recursively.


Okay, I'll discard the all patches, and I'll check the nsPrintEngine's lifetime.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The possible call stack of FlushPendingNotification() in layout/printing are:

PresShell::FlushPendingNotifications()
nsPrintEngine::ReconstructAndReflow()
nsPrintEngine::SetupToPrintContent()
nsPrintEngine::DocumentReadyForPrinting()
nsPrintEngine::AfterNetworkPrint()
nsPrintEngine::InitPrintDocConstruction()
nsPrintEngine::Observe()

This should be safe because when nsIObserver::Observer() is called, it should be grabbed by the caller.

PresShell::FlushPendingNotifications()
nsPrintEngine::ReconstructAndReflow()
nsPrintEngine::SetupToPrintContent()
nsPrintEngine::DocumentReadyForPrinting()
nsPrintEngine::AfterNetworkPrint()
nsPrintEngine::InitPrintDocConstruction()
nsPrintEngine::DoCommonPrint()

This is safe because DoCommonPrint() puts nsScriptSuppressor in the stack and it grabs the nsPrintEngine instance.

PresShell::FlushPendingNotifications()
nsPrintEngine::ReconstructAndReflow()
nsPrintEngine::SetupToPrintContent()
nsPrintEngine::DocumentReadyForPrinting()
nsPrintEngine::AfterNetworkPrint()
nsPrintEngine::OnStateChange()

This should be safe because when nsIWebProgressListener::OnStateChange() is called, it should be grabbed by the caller.

However, nsPrintData::DoOnStatusChange() calls it but it doesn't use nsCOMPtr. So, this should be fixed in this bug.

PresShell::FlushPendingNotifications()
nsPrintEngine::ReconstructAndReflow()
nsPrintEngine::SetupToPrintContent()
nsPrintEngine::DocumentReadyForPrinting()
nsPrintEngine::FinishPrintPreview()
nsPrintEngine::AfterNetworkPrint()

This reaches AfterNetworkPrint(), so, same as above.

PresShell::FlushPendingNotifications()
nsPrintEngine::ReconstructAndReflow()
nsPrintEngine::SetupToPrintContent()
nsPrintEngine::DocumentReadyForPrinting()
nsPrintEngine::FinishPrintPreview()
nsDocumentViewer::Destroy()

It doesn't care recursive calls. And it doesn't grab it without its member. So, this should grab the instance.

PresShell::FlushPendingNotifications()
nsPrintEngine::ReflowPrintObject()
nsPrintEngine::ReflowDocList()
nsPrintEngine::InitPrintDocConstruction()
nsPrintEngine::Observe()

PresShell::FlushPendingNotifications()
nsPrintEngine::ReflowPrintObject()
nsPrintEngine::ReflowDocList()
nsPrintEngine::InitPrintDocConstruction()
nsPrintEngine::DoCommonPrint()

They reach Observe() and DoCommonPrint(). So, same above.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method

Now, these patches are being tested.

These patches can be applied to ESR52 too except this (part.1) due to bug 1342303.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 for ESR52

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 for ESR52

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsPrintEngine::Disconnect() should clear mPrintObj

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8862339 [details] [diff] [review]
part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method

See also comment 18.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8862341 [details] [diff] [review]
part.1 for ESR52

This is similar to the previous patch, but for ESR52.

nsCOMArray cannot be used in range-based for loop. And changing the loop as strictly check *might* be risky for ESR. If you think this should also behave same as the previous patch, let me know. I'll rewrite this. (I think that changing mPrintProgressListeners during the for loops may cause some bugs.)

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8862342 [details] [diff] [review]
part.2 nsPrintEngine::Disconnect() should clear mPrintObj

Review of attachment 8862342 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8862346 [details] [diff] [review]
part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Review of attachment 8862346 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Bug 1354443 part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it r=dholbert

This could be a bit clearer. (In particular, "release" sounds like we're calling ::Release() on mPrintEngine -- but we're not. And the rest is a bit hard to interpret without looking at the code.)

How about something like:
  "While nsDocumentViewer is destroying mPrintEngine, null out its pointer & transfer ownership to local var"
For me at least, that gives me a better high-level idea of what's changing here.

::: layout/base/nsDocumentViewer.cpp
@@ +1731,5 @@
>    // used from JS.
>  
>  #ifdef NS_PRINTING
>    if (mPrintEngine) {
> +    RefPtr<nsPrintEngine> printEngine = mPrintEngine.forget();

If you like, the slightly-more-standard-C++-ish way of doing this would be:
    RefPtr<nsPrintEngine> printEngine = Move(mPrintEngine);

Though this is fine too.

@@ +1741,4 @@
>      }
>  #endif
> +    printEngine->Destroy();
> +    MOZ_ASSERT(!mPrintEngine, "mPrintEngine is recreated during destroying it");

Nit: It's not clear from the text of this message whether the thing it describes ("mPrintEngine is recreated") is the expected behavior or the unexpected behavior.

How about wrapping (to buy you a few more characters) and adjusting to:
    MOZ_ASSERT(!mPrintEngine,
               "mPrintEngine shouldn't be recreated while destroying it");

Comment 29

[Daniel Holbert [:dholbert]]

Just to be sure I'm understanding the point of part 3:
 - You're worried about the possibility of reentrant calls into nsDocumentViewer::Destroy() (via FlushPendingNotifications).
 - And in particular: If we can trigger reentrancy into nsDocumentViewer::Destroy(), then the inner nsDocumentViewer::Destroy call would null out the mPrintEngine pointer, and then the outer one would make a few more function calls on that same member-variable (which is now null), which would make us crash from a null-deref.
 - And your patch saves us from this by effectively making us skip all of the inner nsDocumentViewer::Destroy() code that deals with mPrintEngine.

Do I have that right?

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8862339 [details] [diff] [review]
part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method

Review of attachment 8862339 [details] [diff] [review]:
-----------------------------------------------------------------

[Reviewing part 1 last, because I didn't initially understand what it was doing -- but I do now, I think. :)]

Commit message nit:
> Bug 1354443 part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method r=dholbert

s/at calling/when calling/, perhaps? (or "before calling? The phrase "at calling" doesn't quite make sense.)

::: layout/printing/nsPrintData.cpp
@@ +109,5 @@
>                                  bool         aDoStartStop,
>                                  int32_t      aFlag)
>  {
> +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {

The loop variable "listener" doesn't need to be a full nsCOMPtr here (i.e. it doesn't need to increment the refcount).   We're already holding a local reference via the |listeners| nsCOMArray -- so the extra AddRef/Release for the loop variable is redundant.

So I think this can & should be a C++ reference to a nsCOMPtr -- i.e. just introduce an ampersand like so:
  for (nsCOMPtr<nsIWebProgressListener>& listener : listeners) {

Does that make sense?

@@ +110,5 @@
>                                  int32_t      aFlag)
>  {
> +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {
> +    // Check if the lister still in mPrintProgressListeners.

s/lister/listener/
s/still/is still/

@@ +128,5 @@
>  nsPrintData::DoOnStatusChange(nsresult aStatus)
>  {
> +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {
> +    // Check if the lister still in mPrintProgressListeners.

Same notes as above:
 - The loop variable probably needs a "&"
 - s/lister/listener/
 - s/still/is still/

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8862341 [details] [diff] [review]
part.1 for ESR52

Review of attachment 8862341 [details] [diff] [review]:
-----------------------------------------------------------------

My commit message nits for non-ESR version apply to this ESR version as well, since the commit message is the same.

::: layout/printing/nsPrintData.cpp
@@ +111,5 @@
>                                  bool         aDoStartStop,
>                                  int32_t      aFlag)
>  {
>    for (int32_t i=0;i<mPrintProgressListeners.Count();i++) {
> +    nsCOMPtr<nsIWebProgressListener> wpl = mPrintProgressListeners.ObjectAt(i);

Seems like this should perhaps be SafeElementAt() [1], if you're worried about the array being modified during this loop iteration? (and for consistency with DoOnStatusChange())

(And I presume that's what you're worried about, or else we wouldn't need to bother changing to a nsCOMPtr here.)

[1] or you could use SafeObjectAt(), but that's identical to SafeElementAt() AFAICT, so we might as well stick to using the version we're already using in another related function, for consistency -- so, SafeElementAt.  (I think the parallel SafeObject & SafeElement APIs only exist due to the fact that we've tried to make nsCOMArray more nsTArray-like while preserving a some legacy API names.)

Comment 32

[Daniel Holbert [:dholbert]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #15)
> > Is there a reason you're removing the trailing underscores from the include
> > guard here?  (and in just this .h file and not the other?)
> 
> IIRC, we shouldn't use two or more underscores.
> 
> Additionally, our coding rule defines now here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#CC_practices
> 
> But I'll remove the changes.

Thanks -- I guess the key text in that section of the coding-style guide is around "nsINode.h's guard is nsINode_h".  That convinces me the underscore cleanup is "correct" per the coding style guide. Still: just like misc whitespace cleanup, this doesn't seem like the sort of thing we should fold into an unrelated patch (particularly one whose grokkability is important) -- so, thanks for reverting that part of the change! :)  I'll gladly review them on a separate bug, if you feel like doing that cleanup elsewhere.  (Doesn't seem critical, though.)

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #29)
> Just to be sure I'm understanding the point of part 3:
>  - You're worried about the possibility of reentrant calls into
> nsDocumentViewer::Destroy() (via FlushPendingNotifications).

Yes, and I don't want that destroyed and destroying instance of nsPrintEngine will be referred unexpectedly.

>  - And in particular: If we can trigger reentrancy into
> nsDocumentViewer::Destroy(), then the inner nsDocumentViewer::Destroy call
> would null out the mPrintEngine pointer, and then the outer one would make a
> few more function calls on that same member-variable (which is now null),
> which would make us crash from a null-deref.

Yeah, possible. And if nsPrintEngine::Destroy() will do something which may cause some calls of some methods of nsPrintEngine, we would meet other issues. So, destroying object should be disconnected from owner first.

>  - And your patch saves us from this by effectively making us skip all of
> the inner nsDocumentViewer::Destroy() code that deals with mPrintEngine.

Yes, and avoid using mPrintEngine from any methods of nsDocumentViewer too.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> Comment on attachment 8862339 [details] [diff] [review]
> part.1 nsPrintData should grab nsIWebProgressListener instance in stack at
> calling its method
> ::: layout/printing/nsPrintData.cpp
> @@ +109,5 @@
> >                                  bool         aDoStartStop,
> >                                  int32_t      aFlag)
> >  {
> > +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> > +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {
> 
> The loop variable "listener" doesn't need to be a full nsCOMPtr here (i.e.
> it doesn't need to increment the refcount).   We're already holding a local
> reference via the |listeners| nsCOMArray -- so the extra AddRef/Release for
> the loop variable is redundant.
> 
> So I think this can & should be a C++ reference to a nsCOMPtr -- i.e. just
> introduce an ampersand like so:
>   for (nsCOMPtr<nsIWebProgressListener>& listener : listeners) {
> 
> Does that make sense?

I misunderstand about this issue.

I checked mPrintProgressListeners, then, this is not related to nsPrintEngine. I was thinking that one of this is the owner. However, this comes from outer. I need to investigate where is the caller(s) of nsPrintEngine::OnStateChange(). But anyway, I still think that these loops need to guarantee that the objects in mPrintProgressListeners.  However, it can be unless it grabs its owner.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm... nsPrintEngine::mPrt becomes nullptr before nsPrintEngine is deleted. So, nsPrintData cannot guarantee that mPrintProgressListeners won't be destroyed during nsPrintData::DoOnProgressChange() and nsPrintData::DoOnStatusChange(). So, at least nsPrintData should be refcountable. How do you think?

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I need to investigate where is the caller(s) of nsPrintEngine::OnStateChange().

Okay, the only caller is nsDocLoader::DoFireOnStateChange() which grabs the instance with nsCOMPtr before calling it. So, this path is also safe.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that nsPrintEngine assumes that mPrt won't be released during printing or print preview. But I still don't find where guarantees that.

If it's not guaranteed that nsPrintEngine should grab itself and mPrt instance before calling methods of nsPrintData and check if mPrt is still available after that.

Although, I guess that this doesn't cause any problem actually because perhaps, it's only used by showing the progress bar. However, users of nsPrintEngine can specify any listener to it. That's what I worry about. However, after 57, it must be safe since WebExtentions perhaps cannot overwrite this.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #18)
> PresShell::FlushPendingNotifications()
> nsPrintEngine::ReconstructAndReflow()
> nsPrintEngine::SetupToPrintContent()
> nsPrintEngine::DocumentReadyForPrinting()
> nsPrintEngine::AfterNetworkPrint()
> nsPrintEngine::InitPrintDocConstruction()
> nsPrintEngine::DoCommonPrint()
> 
> This is safe because DoCommonPrint() puts nsScriptSuppressor in the stack
> and it grabs the nsPrintEngine instance.

Hmm, nsScriptSuppressor seems to be different from nsAutoScriptBlocker, so I'm not sure a nsScriptSuppressor makes this safe.

Comment 39

[Timothy Nikkel (:tnikkel)]

As far as script blockers exiting we have at least these

https://dxr.mozilla.org/mozilla-central/rev/2fe636103d7167f3a5d57f61bd19fddcc878ca3c/layout/printing/nsPrintEngine.cpp#515

https://dxr.mozilla.org/mozilla-central/rev/2fe636103d7167f3a5d57f61bd19fddcc878ca3c/layout/printing/nsPrintObject.cpp#101

and anything that calls nsDocumentViewer::DestroyPresShell.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Timothy Nikkel (:tnikkel) from comment #38)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #18)
> > PresShell::FlushPendingNotifications()
> > nsPrintEngine::ReconstructAndReflow()
> > nsPrintEngine::SetupToPrintContent()
> > nsPrintEngine::DocumentReadyForPrinting()
> > nsPrintEngine::AfterNetworkPrint()
> > nsPrintEngine::InitPrintDocConstruction()
> > nsPrintEngine::DoCommonPrint()
> > 
> > This is safe because DoCommonPrint() puts nsScriptSuppressor in the stack
> > and it grabs the nsPrintEngine instance.
> 
> Hmm, nsScriptSuppressor seems to be different from nsAutoScriptBlocker, so
> I'm not sure a nsScriptSuppressor makes this safe.

Ah, that's a good point. I didn't think about script blocker. However, at least the nsPrintEngine is safe because it won't be released, however, it seems that nsPrintData isn't so due to just owned by nsPrintEngine but it's destroyed easier. (Although, if mPrt is cleared, it should crash just referenced to nullptr, or mPrt is recreated, it should just behave odd.)

Perhaps, every method should check mPrt is deleted nor not recreated. And use local variable to refer mPrt in every method which grabs the instance.

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8862346 [details] [diff] [review]
part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Review of attachment 8862346 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Masayuki Nakano [:masayuki] from comment #33)
OK -- r=me on part.3, modulo review feedback in comment 28 (particularly the commit-message & MOZ_ASSERT nits)

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8862341 [details] [diff] [review]
part.1 for ESR52

Review of attachment 8862341 [details] [diff] [review]:
-----------------------------------------------------------------

(Marking part.1 for ESR52 as r- for now, pending your thoughts on switching to SafeElementAt, per comment 31.)

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #34)
> (In reply to Daniel Holbert [:dholbert] from comment #30)
> > Comment on attachment 8862339 [details] [diff] [review]
> > part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method
> > ::: layout/printing/nsPrintData.cpp
> > @@ +109,5 @@
> > >                                  bool         aDoStartStop,
> > >                                  int32_t      aFlag)
> > >  {
> > > +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> > > +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {
> > 
> > The loop variable "listener" doesn't need to be a full nsCOMPtr here
[...]
> 
> I misunderstand about this issue.
> 
> I checked mPrintProgressListeners, then, [...]
> I was thinking that one of this is the owner. However, this
> comes from outer. I need to investigate where is the caller(s) of
> nsPrintEngine::OnStateChange(). But anyway, I still think that these loops
> need to guarantee that the objects in mPrintProgressListeners.  However, it
> can be unless it grabs its owner.

I didn't entirely follow your response here, but I think you might've misunderstood what I was saying.

All I'm saying is: the local variable "listeners" (the nsCOMArray that you're introducing) will AddRef every entry in mPrintProgressListeners and keep it alive for the duration of this function.

So, there's no need to AddRef those entries *again* via the loop's iterator-variable ("listener").  They'll already be kept alive by the nsCOMArray "listeners", as long as we don't modify that array (which we don't do).

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #35)
> Hmm... nsPrintEngine::mPrt becomes nullptr before nsPrintEngine is deleted.
> So, nsPrintData cannot guarantee that mPrintProgressListeners won't be
> destroyed during nsPrintData::DoOnProgressChange() and
> nsPrintData::DoOnStatusChange(). So, at least nsPrintData should be
> refcountable. How do you think?

Sorry, I'm not entirely following this either (in part because I don't entirely recall how all of the functions/objects you mentioned fit together).  But anyway, it sounds like maybe you've moved past this, per end of comment 40... (The bit about mPrt sounds related at least)

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #43)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #34)
> > (In reply to Daniel Holbert [:dholbert] from comment #30)
> > > Comment on attachment 8862339 [details] [diff] [review]
> > > part.1 nsPrintData should grab nsIWebProgressListener instance in stack at calling its method
> > > ::: layout/printing/nsPrintData.cpp
> > > @@ +109,5 @@
> > > >                                  bool         aDoStartStop,
> > > >                                  int32_t      aFlag)
> > > >  {
> > > > +  nsCOMArray<nsIWebProgressListener> listeners(mPrintProgressListeners);
> > > > +  for (nsCOMPtr<nsIWebProgressListener> listener : listeners) {
> > > 
> > > The loop variable "listener" doesn't need to be a full nsCOMPtr here
> [...]
> > 
> > I misunderstand about this issue.
> > 
> > I checked mPrintProgressListeners, then, [...]
> > I was thinking that one of this is the owner. However, this
> > comes from outer. I need to investigate where is the caller(s) of
> > nsPrintEngine::OnStateChange(). But anyway, I still think that these loops
> > need to guarantee that the objects in mPrintProgressListeners.  However, it
> > can be unless it grabs its owner.
> 
> I didn't entirely follow your response here, but I think you might've
> misunderstood what I was saying.
> 
> All I'm saying is: the local variable "listeners" (the nsCOMArray that
> you're introducing) will AddRef every entry in mPrintProgressListeners and
> keep it alive for the duration of this function.

Yeah, but mPrintProgressListeners is a member of the class. That means that they may be removed during a call of an item of it. So, unless grabbing an instance with local variable, it cannot guarantee that the instance won't be released during calling a method.

> So, there's no need to AddRef those entries *again* via the loop's
> iterator-variable ("listener").  They'll already be kept alive by the
> nsCOMArray "listeners", as long as we don't modify that array (which we
> don't do).

So, partially I disagree with this. However, if it can guarantee that any items of mPrintProgressListeners won't be released during a call nor mPrintProgressListeners won't be destroyed (i.e., nsPrintData won't be destroyed), as you say, the loop doesn't need to grab the instance with local variable.

> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #35)
> > Hmm... nsPrintEngine::mPrt becomes nullptr before nsPrintEngine is deleted.
> > So, nsPrintData cannot guarantee that mPrintProgressListeners won't be
> > destroyed during nsPrintData::DoOnProgressChange() and
> > nsPrintData::DoOnStatusChange(). So, at least nsPrintData should be
> > refcountable. How do you think?
> 
> Sorry, I'm not entirely following this either (in part because I don't
> entirely recall how all of the functions/objects you mentioned fit
> together).  But anyway, it sounds like maybe you've moved past this, per end
> of comment 40... (The bit about mPrt sounds related at least)

Yes, now, I think that we should make nsPrintData refcountable again (but nsPrintObject won't be) and nsPrintEngine should stop using mPtr directly. Do you agree with making nsPrintData refcountable?

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-1 Make nsPrintData refcountable

Refreshed patch of attachment 8860348 [details] [diff] [review]. Carrying over the r+.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

This patch uses ugly "Unused << printData" hack for avoiding unused variable warning. I'd like to get rid of this hack with different patch (part.5) or in another bug because it causes a lot of changes and needs unnecessary work for uplifting.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 nsPrintEngine shouldn't use "Unused << printData" hack for avoiding unused variable warnings

So, I think that this should be landed from different bug. However, if you'd like to keep similar code between m-c and ESR52 branch, it's okay to uplift this (especially for other security bugs in layout/printing if there are/will be).

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

2, 3 and 4 don't require specific patches for Beta nor ESR52.

1-1, 1-2 and 5 require specific patches for each Beta and ESR52.

So, either is okay if part 5 should be uplifted.

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8863648 [details] [diff] [review]
part.1-1 Make nsPrintData refcountable

Review of attachment 8863648 [details] [diff] [review]:
-----------------------------------------------------------------

A few notes on the revamped part.1-1:

::: layout/printing/nsPrintEngine.cpp
@@ +868,5 @@
>    *aPrintPreviewNumPages = 0;
>  
>    // When calling this function, the FinishPrintPreview() function might not
>    // been called as there are still some
> +  RefPtr<nsPrintData> prt = mPrtPreview ? mPrtPreview : mPrt;

This change (adding a local RefPtr purely to keep our nsPrintData member-var alive until this function exits) seems like it really belongs in the next patch (part 1.2).

@@ +3354,5 @@
>      // printing.
>      return;
>    }
>  
> +  RefPtr<nsPrintData> prt = mPrt;

Same here.

::: layout/printing/nsPrintEngine.h
@@ +264,5 @@
>    nsCOMPtr<nsIDocumentViewerPrint> mDocViewerPrint;
>    nsWeakPtr               mContainer;
>    float                   mScreenDPI;
>  
> +  RefPtr<nsPrintData> mPrt;

This change (UniquePtr --> RefPtr) makes ownership a bit harder to understand here.

Could add a comment to make the ownership (& reason for refcounting) clearer here? I'm imagining something like:

  // We are the primary owner of our nsPrintData member vars.  These vars
  // are refcounted so that functions (e.g. nsPrintData methods) can create
  // temporary owning references when they need to fire a callback that
  // could conceivably destroy this nsPrintEngine owner object and all its
  // member-data.

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 8863649 [details] [diff] [review]
part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

Review of attachment 8863649 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/printing/nsPrintEngine.cpp
@@ +617,5 @@
>          //
>          // ShowPrintDialog triggers an event loop which means we can't assume
>          // that the state of this->{anything} matches the state we've checked
>          // above. Including that a given {thing} is non null.
> +        if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {

|printData| is guaranteed to be non-null (because of infallible allocation), so your first condition here is unnecessary.  That first condition (mPrt being null) is just a special case of the second condition (mPrt differing from printData).

So I think you can remove the first condition -- and this can just be simplified to:
    if (NS_WARN_IF(mPrt != printData)) {

@@ +1037,5 @@
>    }
>  
> +  // Guarantee that any objects owned by mPrt won't be released.
> +  RefPtr<nsPrintData> printData = mPrt;
> +  Unused << printData;

Could you adjust this comment to make it clearer *why/where* we think they might be released?  (Is it GetShowPrintProgress? And is it because we spin the event loop, or flush layout, or what?)

Also: please use the word "deleted" or "freed" instead of "released", throughout the comments in this patch.   ("Released" sounds like it just means "Release() was called", and you can't provide any guarantees about that.  Indeed, it's *the fact that Release() may get called* that is prompting us to add new RefPtrs here.)

@@ +1593,5 @@
>    }
>  #endif
>  
> +  // In this loop, nsPrintData shouldn't be released.  So, make it guarantee
> +  // with local variable.

"nsPrintData shouldn't be" is too ambiguous here. (It can be read as meaning "nothing to worry about, it shouldn't happen")

Perhaps replace with:
  // In this loop, we use mPrt a lot, but mPrt might get stomped on in a flush.
  // So we maintain a local owning reference to mPrt, which we use instead.

@@ +1614,4 @@
>      po->mPresContext->SetPrintPreviewScale(mScreenDPI / printDPI);
>  
>      po->mPresShell->ReconstructFrames();
> +    if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {

As above, the first condition here is unnecessary, because it's a special-case of the second (because we're already depending on |printData| being non-null).

Also, please add a comment to explain why we're checking here in particular.  (I think it's because the ReconstructFrames call just before this flushes notifications, including e.g. nsIMutationObserver notifications, and hence can cause arbitrary script to run.)

@@ +1633,5 @@
>        }
>      }
>  
>      po->mPresShell->FlushPendingNotifications(FlushType::Layout);
> +    if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {

Here as well, first condition seems unnecessary, and a comment would be helpful like "If that flush destroyed/modified us, then bail."

@@ +1728,3 @@
>      mPrt->OnStartPrinting();
> +    if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {
> +      return NS_ERROR_FAILURE;

Here as well, first condition seems unnecessary, and a comment would be helpful like "If OnStartPrinting triggered some JS that destroyed/modified us, then bail."

@@ +1844,5 @@
>  nsresult
>  nsPrintEngine::InitPrintDocConstruction(bool aHandleError)
>  {
>    nsresult rv;
> +  // Guarantee that mPrintObject won't be released.

s/mPrintObject/mPrt->mPrintObject/  (or maybe "the nsPrintData's mPrintObject")

("mPrintObject" is not a valid variable here.)

@@ +2141,5 @@
>  
>    NS_ASSERTION(!aPO->mPresContext, "Recreating prescontext");
>  
> +  // Guarantee that any objects owned by mPrt won't be released.
> +  RefPtr<nsPrintData> printData = mPrt;

This is a bit confusing because it looks like mPrt & printData aren't used at all here.

Perhaps extend the comment slightly with:
 s/owned by mPrt/owned by mPrt (e.g. aPO)/

(I'm assuming that's the main object you're caring about here, and that you've checked that it is owned by mPrt)

@@ +2430,5 @@
>    NS_ASSERTION(poPresContext->Type() != nsPresContext::eContext_PrintPreview,
>                 "How did this context end up here?");
>  
> +  // Guarantee that any objects owned by mPrt won't be released.
> +  RefPtr<nsPrintData> printData = mPrt;

Same here.

@@ +2767,5 @@
> +  RefPtr<nsPrintData> printData = mPrt;
> +  printData->DoOnProgressChange(++printData->mNumPagesPrinted,
> +                                endPage, false, 0);
> +  if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {
> +    return true;

Per above, the first condition seems unnecessary, and an explanatory comment would be helpful to explain why we're bailing.

Also, it would be useful to document why we're doing "return true" for this error condition. (After a bit of research, I think it's because (a) return true means we're done printing in this particular function, (2) if we get here, something is horribly wrong, and so we do want to indicate that we're done printing so that no further PrintPage calls come in.)

@@ +3173,5 @@
>  nsPrintEngine::EnablePOsForPrinting()
>  {
> +  // Guarantee that any objects owned by mPrt won't be released.
> +  RefPtr<nsPrintData> printData = mPrt;
> +  Unused << printData;

What precisely are we worried might happen in this function -- is it that mPrt might get stomped on? (and that's how its objects would be released?)

If so, then I think we need to upgrade all mPrt usages to |printData| instead. (You've done that in other parts of this patch, but here, you've split it out into a separate patch [part 5] for some reason -- I'm not sure if that was because of a special difference here or not.  Don't we need it here just as much as we need it elsewhere?)

(FWIW, it seems that our best practice is to prefer using the "deathgrip" variable (printData in this case) rather than the original copy, per https://bugzilla.mozilla.org/show_bug.cgi?id=1018486#c1 .)

@@ +3503,5 @@
>       * what went wrong...
>       */
> +    printData->OnEndPrinting();
> +    // XXX mPrt may be nullptr here.  So, Shouldn't TurnScriptingOn() take
> +    //     nsPrintData as an argument?

Hmm. If scripting was really off (as it sounds like it was -- presumably that's why we call TurnScriptingOn right after this), are you sure it's really possible for mPrt to be nullptr?  I assume the way for mPrt to get stomped would be from OnEndPrinting triggering a callback of some sort -- but if scripting is disabled, then we perhaps can make some stronger assumptions about what sorts of things that callback could do.

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8863652 [details] [diff] [review]
part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it

Review of attachment 8863652 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8863653 [details] [diff] [review]
part.5 nsPrintEngine shouldn't use "Unused << printData" hack for avoiding unused variable warnings

Review of attachment 8863653 [details] [diff] [review]:
-----------------------------------------------------------------

I'm canceling review on part.5 for now since I'm not yet clear why it's separated from part.1-2, as noted in my comment above on EnablePOsForPrinting.

Comment 55

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #52)
> Hmm. If scripting was really off (as it sounds like it was -- presumably
> that's why we call TurnScriptingOn right after this), are you sure it's
> really possible for mPrt to be nullptr?  I assume the way for mPrt to get
> stomped would be from OnEndPrinting triggering a callback of some sort --
> but if scripting is disabled, then we perhaps can make some stronger
> assumptions about what sorts of things that callback could do.

Note that TurnScriptingOn just seems to turn off content js. It doesn't stop scripts in the sense of nsAutoScriptBlocker.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #52)
> ::: layout/printing/nsPrintEngine.cpp
> @@ +617,5 @@
> >          //
> >          // ShowPrintDialog triggers an event loop which means we can't assume
> >          // that the state of this->{anything} matches the state we've checked
> >          // above. Including that a given {thing} is non null.
> > +        if (NS_WARN_IF(!mPrt) || NS_WARN_IF(mPrt != printData)) {
> 
> |printData| is guaranteed to be non-null (because of infallible allocation),
> so your first condition here is unnecessary.  That first condition (mPrt
> being null) is just a special case of the second condition (mPrt differing
> from printData).
> 
> So I think you can remove the first condition -- and this can just be
> simplified to:
>     if (NS_WARN_IF(mPrt != printData)) {

Oh, tricky, but fine.

> @@ +1037,5 @@
> >    }
> >  
> > +  // Guarantee that any objects owned by mPrt won't be released.
> > +  RefPtr<nsPrintData> printData = mPrt;
> > +  Unused << printData;
> 
> Could you adjust this comment to make it clearer *why/where* we think they
> might be released?  (Is it GetShowPrintProgress? And is it because we spin
> the event loop, or flush layout, or what?)

I assume that from "anything". If it's not guaranteed, the methods may access deleted memory accidentally. nsPrintEngine should always guarantee the lifetime of objects when it's nsPrintData or some objects which owned by nsPrintData. It's dangerous if we depend on current each method's implementation. So, I'll add comments which method needs the guarantee.

> Also: please use the word "deleted" or "freed" instead of "released",
> throughout the comments in this patch.   ("Released" sounds like it just
> means "Release() was called", and you can't provide any guarantees about
> that.  Indeed, it's *the fact that Release() may get called* that is
> prompting us to add new RefPtrs here.)

Sure.

> @@ +3173,5 @@
> >  nsPrintEngine::EnablePOsForPrinting()
> >  {
> > +  // Guarantee that any objects owned by mPrt won't be released.
> > +  RefPtr<nsPrintData> printData = mPrt;
> > +  Unused << printData;
> 
> What precisely are we worried might happen in this function -- is it that
> mPrt might get stomped on? (and that's how its objects would be released?)

mPrt->mPrintObject is passed to other methods. So, I think that it should just guarantee the lifetime (perhaps, they are safe, but we can say "at least for now").

> If so, then I think we need to upgrade all mPrt usages to |printData|
> instead. (You've done that in other parts of this patch, but here, you've
> split it out into a separate patch [part 5] for some reason -- I'm not sure
> if that was because of a special difference here or not.  Don't we need it
> here just as much as we need it elsewhere?)

So, in other words, it's not urgent for now. So, not necessary to uplift. Patches which should be uplifted should be simpler and easier to merge for avoiding to merge by hand.

> (FWIW, it seems that our best practice is to prefer using the "deathgrip"
> variable (printData in this case) rather than the original copy, per
> https://bugzilla.mozilla.org/show_bug.cgi?id=1018486#c1 .)

Yes, I completely agree with the comment. However, changing a lot of lines may require to merge by hand because there are a lot of whitespace changes at fixing bug 1342994 (https://reviewboard.mozilla.org/r/117546/diff/2#index_header). Therefore, I don't want to do it in uplifting patches for avoiding any mistake.

> @@ +3503,5 @@
> >       * what went wrong...
> >       */
> > +    printData->OnEndPrinting();
> > +    // XXX mPrt may be nullptr here.  So, Shouldn't TurnScriptingOn() take
> > +    //     nsPrintData as an argument?
> 
> Hmm. If scripting was really off (as it sounds like it was -- presumably
> that's why we call TurnScriptingOn right after this), are you sure it's
> really possible for mPrt to be nullptr?

No, not sure. I just want to kill the possible points of security issues or crash bugs.

> I assume the way for mPrt to get
> stomped would be from OnEndPrinting triggering a callback of some sort --
> but if scripting is disabled, then we perhaps can make some stronger
> assumptions about what sorts of things that callback could do.

nsPrintData::OnEndPrinting() calls interface methods. That means that it's not managed by layout/printing code what will happen during a call. So, we shouldn't assume it's safe.

(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #54)
> Comment on attachment 8863653 [details] [diff] [review]
> part.5 nsPrintEngine shouldn't use "Unused << printData" hack for avoiding
> unused variable warnings
> 
> Review of attachment 8863653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm canceling review on part.5 for now since I'm not yet clear why it's
> separated from part.1-2, as noted in my comment above on
> EnablePOsForPrinting.

So, I'm thinking that the safest way to uplift is, only uplifting part.1-1 ~ part.4. However, this causes different code base between 55 and ESR52.  Therefore, it will cause making fixing other security bugs in nsPrintEngine harder. This is also not good.  So, if you think that the part.5 should be uplifted to ESR52, I can merge part.1-2 and part.5 (but it might become unclear where are the problem).

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-1 Make nsPrintData refcountable

Moved the two changes to next patch.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

Okay, merged part.1-2 and part.5. Let's take all changes to ESR52 if it'd be allowed.

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I added a lot of comments into part.1-2. However, if some of them unclear or I forgot to add some comments which you addressed, let me know.

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

Fix some nits.

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-1 for Beta

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 for Beta

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-1 for ESR52

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 for ESR52

Comment 65

[Daniel Holbert [:dholbert]]

Comment on attachment 8866248 [details] [diff] [review]
part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

Review of attachment 8866248 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits:

First, two commit message nits:
 (1) Ideally you should wrap the explanatory part of the commit message ("For example ...") to 80 characters.  (Of course, the commit message's first line needs to all be on a single line, regardless of how long it is, because the first line is semantically special.  But the explanatory stuff afterwards has no special semantic difference between its multiple lines, so can & should be wrapped like any other README/code/tests/etc.)

 (2) One wording nits on that explanatory part [which I'll wrap here myself, for Bugzilla's benefit]:
> For example, mPrt->mPrintObject is owned by mPrt.  When nsPrintEngine
> [...]
> mPrt->mPrintObject won't be deleted during the method is called.

Grammar nit: s/method is called/method-call/

::: layout/printing/nsPrintEngine.cpp
@@ +1051,5 @@
>      showProgresssDialog = Preferences::GetBool("print.show_print_progress");
>    }
>  
> +  // Guarantee that mPrt and its owning objects won't be deleted.  If this
> +  // method shows the progress, anything can occur.  So, mPrt may be cleared

Two changes for this comment:
  (1) s/its owning objects/the objects it owns/

(The phrase "its owning objects" is easy to misinterpret. To me, it sounds like it might mean "its owners", but that's the opposite of what you mean)

  (2) s/shows the progress/shows a progress dialog & spins the event loop/

(Right now it's unclear how/why "anything can occur".  I think it's specifically the dialog [and the event-loop-spinning in the background when the dialog appears] that enables anything to occur -- right?)

@@ +1621,5 @@
>    }
>  #endif
>  
> +  // In this loop, mPrt shouldn't be deleted for guaranteeing that accessing it
> +  // safe.  Therefore grab it with local variable.

This comment doesn't quite make grammatical sense, so it's hard for me to understand what it's saying.

I think you mean something like:
  // In this loop, it's conceivable that one of our helpers might clear mPrt,
  // while we're using it & its members!  So we capture it in an owning local
  // reference & use that instead of using mPrt directly.

Please reword like that ^^ or something else that's similarly clear.

@@ +1689,5 @@
>    bool didReconstruction = false;
>  
> +  // This method works with mPrt->mPrintObject.  So, this should guarantee that
> +  // it won't be deleted in this method.  mPrt->mPrintObject is owned by mPrt.
> +  // Therefore, mPrt should be grabbed by a local variable.

This should be reworded a bit -- the wording & directionality of the logic is a bit confusing here. (Specifically, "this should guarantee" sounds a bit like "the fact stated in the previous sentence can be assumed to guarantee" -- but that's not what you want to say.)

So anyway, two requested wording changes here:
 (1) s/this should guarantee/we need to guarantee/
 (2) s/mPrt->mPrintObject is owned [...] mPrt should be grabbed by a local variable/
      We achieve this by holding a strong local reference to mPrt, which in turn keeps mPrintObject alive./

@@ +1703,5 @@
> +    if (NS_WARN_IF(mPrt != printData)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;

I suspect we should flip the order of these two error-handling early returns -- we should handle NS_FAILED(rv) first, and then *after that* we should check for mPrt != printData.

(If our call to ReconstructAndReflow() produced a useful failure code, and also happened to also stomp on mPrt, then I think we'd want to propagate the useful failure code up our caller chain, rather than simply ignoring |rv| and returning NS_ERROR_FAILURE.  The new mPrt!=printData check makes sense to me as an *additional* sanity-check, to intervene & stop us from proceeding when things otherwise look successful -- but I don't think we need to preempt other error codes.  If you disagree, we should perhaps justify that with a brief code-comment explaining why we're disregarding |rv|.)

@@ +1734,5 @@
> +      // let's stop doing this printing.
> +      if (NS_WARN_IF(mPrt != printData)) {
> +        return NS_ERROR_FAILURE;
> +      }
> +      if (NS_WARN_IF(NS_FAILED(rv))) {

Same here -- it seems to me the NS_FAILED(rv) check should happen before the mPrt check.

@@ +2204,5 @@
>    }
>  
>    NS_ASSERTION(!aPO->mPresContext, "Recreating prescontext");
>  
> +  // Guarantee that mPrt and its owning objects won't be deleted in this method

As above, s/its owning objects/the objects it owns/

@@ +2494,5 @@
>    NS_ASSERTION(poPresContext, "PrintObject has not been reflowed");
>    NS_ASSERTION(poPresContext->Type() != nsPresContext::eContext_PrintPreview,
>                 "How did this context end up here?");
>  
> +  // Guarantee that mPrt and its owning objects won't be deleted in this method

As above, s/its owning objects/the objects it owns/

@@ +2739,5 @@
>  bool
>  nsPrintEngine::PrePrintPage()
>  {
> +  MOZ_ASSERT(mPageSeqFrame.IsAlive());
> +  MOZ_ASSERT(mPrt);

Let's revert this ^^ change to these assertions.

I agree with some *other* assertion-strengthening that you're doing in this patch, since in those cases, we're clearly about to crash if the asserted condition were to fail (via null deref of the thing that's being asserted about) -- so we might as well make debug builds crash more usefully & slightly earlier.  

But in *this case here*, the existing NS_ASSERTION seems reasonably-appropriate because we can usefully proceed even if the assertion fails, via the early return right after this.  So: since I don't immediately know how justified we are in upgrading this assertion, I'd rather leave these untouched. (See also http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html for why nonfatal assertions are often preferable, in cases where we can usefully proceed after assertion-failure.)

And on top of that, these particular assertions aren't really tied to the main thrust of this patch (adding owning pointers).  So since the justification for upgrading them is non-obvious [to me at least], I'd rather not group them as part of this patch.

@@ +2782,5 @@
>                           bool&           aInRange)
>  {
> +  MOZ_ASSERT(aPO);
> +  MOZ_ASSERT(mPageSeqFrame.IsAlive());
> +  MOZ_ASSERT(mPrt);

Here as well, please don't upgrade these assertions (at least not as part of this patch).  We already have logic (below this) to let us usefully proceed if these assertions were to fail, so it's not clear we benefit from making these assertions fatal.

If you'd really like to upgrade them, let's do that separately & with more analysis/justification on why the assertions are justified.

@@ +2793,5 @@
>    }
>  
> +  // Guarantee that mPrt won't be deleted during a call of
> +  // nsPrintData::DoOnProgressChange() which runs some listeners,
> +  // that may cause destroying mPrt.

Grammar & clarity nit:
  s/that may cause destroying/which may clear (& might otherwise destroy)/

@@ +3182,5 @@
>      pageSeqFrame->ResetPrintCanvasList();
>    }
>  
> +  // Guarantee that mPrt and mPrt->mPrintObject won't be deleted during a
> +  // call of PrintDocContent() and FirePrintCompletionEvent(). 

Trailing whitespace at the end of this comment.

@@ +3199,5 @@
>    if (NS_SUCCEEDED(aResult)) {
>      FirePrintCompletionEvent();
> +    // XXX mPrt may be cleared or replaced with new instance here.
> +    //     However, following methods will clean up with new mPrt or does
> +    //     nothing due to no proper nsPrintData instance.

s/following/the following/
s/or does/or will do/

@@ +3259,5 @@
>  //-------------------------------------------------------
>  nsresult
>  nsPrintEngine::EnablePOsForPrinting()
>  {
> +  // Guarantee that mPrt and its owning objects won't be deleted.

As above, s/its owning objects/the objects it owns/

@@ +3505,5 @@
>      // printing.
>      return;
>    }
>  
> +  // Following for loop use nsPrintObject instances which are owned by

"The following for loop uses nsPrintObject instances that are [etc]"

@@ +3506,5 @@
>      return;
>    }
>  
> +  // Following for loop use nsPrintObject instances which are owned by
> +  // mPrt or mPrtPreview.  Therefore, this method should guarantee that

s/This method should/This method needs to/

(As noted above, "should guarantee" is ambiguous -- it can be mistakenly interpreted as "can be assumed to guarantee", but really you want to say that it "needs to guarantee")

@@ +3594,5 @@
>  
>    SetIsCreatingPrintPreview(false);
>  
> +  // mPrt may be cleared during a call of nsPrintData::OnEndPrinting()
> +  // because it calls some listeners which is not managed by this module.

Let's just simplify the last line to:
  // because that method invokes some arbitrary listeners.

("not managed by this module" doesn't seem like a particularly useful clarification -- even if they were managed by this module, I'm not convinced they'd be guaranteed to leave things in a coherent state. :) )

Comment 66

[Daniel Holbert [:dholbert]]

Note: I'm assuming there's no known/obvious way to remotely exploit the things that this bug is fixing, since (a) printing requires user interaction and (b) a lot of the possible issues here seem to be from callbacks being invoked during printing, which probably would require an attacker to be setting up privileged code.

If I'm correct in this assumption, then I think we're OK to include all of your helpful explanations in the commit message & comments.  But if you think this might be exploitable by an unprivileged attacker & a user printing that attacker's page, we should strongly consider reverting all [or most of] the code-comments & landing them in a followup patch after all branches have gotten the fix, to avoid pointing too much of a spotlight on targets when we land this on trunk.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you very much! And I'm very happy because of you explained how native speakers understand my wrong words. It really helps me.


(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #66)
> Note: I'm assuming there's no known/obvious way to remotely exploit the
> things that this bug is fixing,

Yes.

> since (a) printing requires user interaction
> and (b) a lot of the possible issues here seem to be from callbacks being
> invoked during printing, which probably would require an attacker to be
> setting up privileged code.

Yes. I guess that make users install addon which listens the progress listeners is the simplest entrance. However, it's not critical. On the other hand, there are some crash reports crashed in nsPrintEngine, nsPrintData and nsPrintObject:

https://crash-stats.mozilla.com/search/?signature=~nsPrintEngine&date=%3E%3D2016-11-12T04%3A31%3A29.000Z&date=%3C2017-05-12T04%3A31%3A29.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
https://crash-stats.mozilla.com/search/?signature=~nsPrintData&date=%3E%3D2016-11-12T04%3A31%3A29.000Z&date=%3C2017-05-12T04%3A31%3A29.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
https://crash-stats.mozilla.com/search/?signature=~nsPrintObject&date=%3E%3D2016-11-12T04%3A31%3A29.000Z&date=%3C2017-05-12T04%3A31%3A29.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Some of them could be fixed with this patch. If so, there may be some attacking way without specific addons.

> If I'm correct in this assumption, then I think we're OK to include all of
> your helpful explanations in the commit message & comments.  But if you
> think this might be exploitable by an unprivileged attacker & a user
> printing that attacker's page, we should strongly consider reverting all [or
> most of] the code-comments & landing them in a followup patch after all
> branches have gotten the fix, to avoid pointing too much of a spotlight on
> targets when we land this on trunk.

So, I think that we can include the comments when we land them.


mccr8:

Do you still think that this is sec-high? And how do you think that if these patches should include the comments. (Anyway, even without the comments, attackers may be able to understand *why* we need these fixes.)

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 for Beta

Comment 70

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1-2 for ESR52

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, and this could fix bug 1354447 which is also marked as sec-high.

Comment 72

[Andrew McCreight [:mccr8]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #67)
> Do you still think that this is sec-high? And how do you think that if these
> patches should include the comments. (Anyway, even without the comments,
> attackers may be able to understand *why* we need these fixes.)

I think sec-moderate is fine, because we're only triggering chrome JS code. The comments are fine. Like you said, I think it is obvious enough what you are doing anyways even without the comments. If bug 1354447 is also only printing related, then you could make that sec-moderate as well. Thanks for working on fixing this!

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Andrew McCreight [:mccr8] from comment #72)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #67)
> > Do you still think that this is sec-high? And how do you think that if these
> > patches should include the comments. (Anyway, even without the comments,
> > attackers may be able to understand *why* we need these fixes.)
> 
> I think sec-moderate is fine, because we're only triggering chrome JS code.
> The comments are fine. Like you said, I think it is obvious enough what you
> are doing anyways even without the comments. If bug 1354447 is also only
> printing related, then you could make that sec-moderate as well. Thanks for
> working on fixing this!

Thanks. I'll land them to m-c soon. After that, I'll try to request approvals to uplift them. (Although, they'd be not granted due to the risk.)

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23f739101451107f990cfa1ff380a855d384610a
Bug 1354443 part.1-1 Make nsPrintData refcountable r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1b8abc499e2a86c5912e54fac443c5758ba914
Bug 1354443 part.1-2 Methods of nsPrintEngine should guarantee that objects owned by nsPrintData won't be released when they're referred by calling methods via their arguments r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d384ede2364c24219815663f142a922867c84fc
Bug 1354443 part.2 nsPrintEngine::Disconnect() should clear mPrintObj r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/2329af476fa639074778fd849bea6e205a904505
Bug 1354443 part.3 While nsDocumentViewer is destroying mPrintEngine, null out its pointer & transfer ownership to local var r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/82bc09e9f1914f55ad37920fec55503bc6ff408b
Bug 1354443 part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it r=dholbert

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23f739101451
https://hg.mozilla.org/mozilla-central/rev/4e1b8abc499e
https://hg.mozilla.org/mozilla-central/rev/8d384ede2364
https://hg.mozilla.org/mozilla-central/rev/2329af476fa6
https://hg.mozilla.org/mozilla-central/rev/82bc09e9f191

I definitely think we'll want QA to do some printing smoketesting before we consider this for any uplifts.

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #75)
> I definitely think we'll want QA to do some printing smoketesting before we
> consider this for any uplifts.

Do I need to do something to request to QA team for smoketesting by them?

Comment 77

[Ryan VanderMeulen [:RyanVM]]

I could have sworn I'd already set qe-verify+ on this, but apparently not. Redirecting to Rares to see if someone on his team can do some printing smoketesting on a current Nightly.

Comment 78

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi Masayuki,

Before we start testing, can you please provide more details about the areas that need to be tested for this issue? 
Also, any test cases and expected results (and where to see them) would be of great help.

Thanks!

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Cosmin Muntean [:CosminMCG], Desktop Engineering QA from comment #78)
> Before we start testing, can you please provide more details about the areas
> that need to be tested for this issue? 
> Also, any test cases and expected results (and where to see them) would be
> of great help.

Hi! Unfortunately, there is no actual plan what should be tested.

The patches touched almost all paths of print preview and printing. So, if you have some basic tests about both of print preview and printing, that's grate to check if the patches broke something.

Note that I didn't touch about rendering of the pages except <iframe> management. So, I'd be happy if you check if:
* Print preview works as expected in any pages (especially with the pages which have some <iframe>s)
* Printing should work as same as print preview (I think it's enough to printing to PDF or something)

Comment 80

[Abe - QA (:Abe_LV)]

Verified fixed.
Our test runs show green except one which is not caused by this fix.
The new bug is 1366904. Test summary is available here: https://drive.google.com/a/mozilla.com/file/d/1C9x2HyRuoBkxJBl524_-vrCWe020n_NkUX5QRGpmA8HzsHD6eOA1h3QBT9OSyngWHZyfzpvXXwx-tsK6/view?usp=sharing

Let me know if you have questions or feedback.
Thanks

Comment 81

[Nathan Froyd [:froydnj]]

If the test results look good, could you please request uplift on this?  It's a little late in the beta cycle, but we should at least have the discussion.

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, oddly, I cannot access the URL due to "G Suite Server Error" since today's morning (even with my MoCo account). Are you sure to make it sharing?

Comment 83

[Abe - QA (:Abe_LV)]

Attachment: Manual Testing Summary- Print and print preview smoke test on 55.0a1(2017-05-22) - TestRail.pdf

The link is shared again. The document is also attached here.

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Abe - QA (:Abe_LV) from comment #83)
> Created attachment 8871387 [details]
> Manual Testing Summary- Print and print preview smoke test on
> 55.0a1(2017-05-22) - TestRail.pdf
> 
> The link is shared again. The document is also attached here.

Oops, sorry for the delay. But I cannot log in to TestRail with my email address of mozilla.com. And cannot use "I forgot my password" since it claims that my email address is unknown. Therefore, I cannot check each item what you did. However, according to each test summary. You did enough test. So, I bet that we can uplift the patches.

Thank you!

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8866249 [details] [diff] [review]
part.1-1 for Beta

Approval Request Comment
[Feature/Bug causing the regression]:

Long standing bug.

[User impact if declined]:

Might crash with use-heaf-after-free. (Actually, we have some crash reports.)

[Is this code covered by automated tests?]:

No.

[Has the fix been verified in Nightly?]:

Yes, and QA team tested the printing behavior.

[Needs manual test from QE? If yes, steps to reproduce]: 

No, but if we uplift them, QA team should check printing features entirely.

[List of other uplifts needed for the feature/fix]:

May require https://hg.mozilla.org/mozilla-central/rev/3332a33334d0 which cleaned up only whitespaces.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

Just making nsPrintData refcountable.

[String changes made/needed]:

No.

(I think that it's too late for Beta for this bug but I shouldn't decide it, it should be decided by release drivers.)

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8867130 [details] [diff] [review]
part.1-2 for Beta

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:

See above.

[Is the change risky?]:

No, but changed a lot of lines.

[Why is the change risky/not risky?]:

This patch just changes lines which calls members' methods directly to use local strong pointer.

[String changes made/needed]:

No.

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8862342 [details] [diff] [review]
part.2 nsPrintEngine::Disconnect() should clear mPrintObj

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:

See above.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

Because this clears a raw pointer which may be outdated.

[String changes made/needed]:

No.

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8863651 [details] [diff] [review]
part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:

See above.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

Making nsDocumentViewer::Destroy() use local strong pointer to call nsPrintEngine::Destroy().

[String changes made/needed]:

No.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8863652 [details] [diff] [review]
part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:

See above.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

This patch makes nsPrintData use local strong pointer to call methods of nsIWebProgressListener.

[String changes made/needed]:

No.

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8866252 [details] [diff] [review]
part.1-1 for ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Perhaps, for using this bug to attack, they need to create or existing add-on which can cause this crash bug. But there are some heap-use-after-free crash bugs. So, I recommend to uplift them. (Unfortunately, printing function may not be used by Beta/Nightly testers. Most crashes are reported by Release users. Therefore, I'm still not sure how many crashes are fixed with these patches.)

User impact if declined: 

May meet heap-use-after-free crashes.

Fix Landed on Version:

55.

Risk to taking this patch (and alternatives if risky):

Not risky since this makes nsPrintData refcountable.

String or UUID changes made by this patch:

No.



These patches require to uplift https://hg.mozilla.org/mozilla-central/rev/3332a33334d0 which clean up whitespaces.

Comment 91

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8867131 [details] [diff] [review]
part.1-2 for ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:

See above.

Risk to taking this patch (and alternatives if risky): 

Not so risky even though this changes a lot of lines since they just stop using member variables to call their method.  Instead, using local strong variables.

String or UUID changes made by this patch:

No.

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8862342 [details] [diff] [review]
part.2 nsPrintEngine::Disconnect() should clear mPrintObj

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:

See above.

Risk to taking this patch (and alternatives if risky): 

Not risky. This just clean up a raw pointer which may be outdated if referred later.

String or UUID changes made by this patch: 

No.

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8863651 [details] [diff] [review]
part.3 nsDocumentViewer should release mPrintEngine first and keep grabbing it in stack during destroying it

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:

See above.

Risk to taking this patch (and alternatives if risky): 

Not risky. Just using local strong pointer when nsDocumentViewer destroys mPrintEngine.

String or UUID changes made by this patch: 

No.

Comment 94

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8863652 [details] [diff] [review]
part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:

See above.

Risk to taking this patch (and alternatives if risky): 

Not risky because this patch makes nsPrintData call methods of nsIWebProgressListener with local strong pointer.

String or UUID changes made by this patch: 

No.

Comment 95

[Liz Henry (:lizzard) (relman/hg->git project)]

We are doing the RC build for 54 today, let's keep this fix on 55 as it is just a bit too last minute.

Comment 96

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8863652 [details] [diff] [review]
part.4 nsPrintData should guarantee that object which has nsIWebProgressListener interface won't be released during calling a method of it

Ryan suggested leaving the esr52 uplift requests in place for 52.3.0esr. 
Good idea. I'll reset the relevant flags.

Comment 97

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8867131 [details] [diff] [review]
part.1-2 for ESR52

Approving for ESR52.3 now since this has been waiting. Can we get this checked into the ESR52 branch?

Comment 98

[Ryan VanderMeulen [:RyanVM]]

The ESR52 part.1-1 patch has conflicts.

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #98)
> The ESR52 part.1-1 patch has conflicts.

Did you graft https://hg.mozilla.org/mozilla-central/rev/3332a33334d0 first as I said in comment 90?

Comment 100

[Ryan VanderMeulen [:RyanVM]]

I missed that, sorry.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8c27a68ee87e
https://hg.mozilla.org/releases/mozilla-esr52/rev/b2ee0c5466da
https://hg.mozilla.org/releases/mozilla-esr52/rev/25f6ec16e501
https://hg.mozilla.org/releases/mozilla-esr52/rev/30443b4f758e
https://hg.mozilla.org/releases/mozilla-esr52/rev/1b934ab92c59
https://hg.mozilla.org/releases/mozilla-esr52/rev/830a345dc0e7

Comment 102

[Ciprian Georgiu, Desktop QA]

I performed smoke tests around printing functionality, covering most of the test scenarios from comment 83. No issues were encountered during testing on 55.0b13 (20170727114534) with Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6.

Marking this issue as verified fixed on 55 Beta as well.

Comment 103

[Jonathan Watt [:jwatt]]

Looks like this needs landing on ESR.

Comment 104

[Jonathan Watt [:jwatt]]

Hmm, I guess that already happened - a bit confusing that the approvals were marked a week after the ESR landing...

Comment 105

[Ryan VanderMeulen [:RyanVM]]

To be clear, comment 97 is where the main ESR52 approval took place. The later ones were just flag cleanups.

Comment 106

[Ciprian Georgiu, Desktop QA]

This is also verified fixed on 52.3.0 esr (20170802111520) running Win 10 x64, macOS 10.12 and Ubuntu 16.04 x64 LTS.