Closed Bug 1376693 Opened 7 years ago Closed 7 years ago

Crash in nsPrintEngine::SetupToPrintContent

Categories

(Core :: Print Preview, defect)

Product:
Core
Component:
Print Preview
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1376693 - part2: Make nsPrintEngine::DoCommonPrint() stop initializing the instance when the owner stops using the instance
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
This bug was filed from the Socorro interface and is 
report bp-7c2e19a9-1e3e-4308-b80c-b14b90170627.
=============================================================
>  0 	xul.dll	nsPrintEngine::SetupToPrintContent()
> 1 	xul.dll	nsPrintEngine::DocumentReadyForPrinting()
> 2 	xul.dll	nsPrintEngine::FinishPrintPreview()
> 3 	xul.dll	nsDocumentViewer::Destroy()
> 4 	xul.dll	nsDocShell::Destroy()
> 5 	xul.dll	nsWebBrowser::SetDocShell(nsIDocShell*)
> 6 	xul.dll	nsWebBrowser::InternalDestroy()
> 7 	xul.dll	nsWebBrowser::Destroy()
> 8 	xul.dll	mozilla::dom::TabChild::DestroyWindow()
> 9 	xul.dll	mozilla::dom::TabChild::RecvDestroy()
> 10 	xul.dll	mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&)
> 11 	xul.dll	mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)
> 12 	xul.dll	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)
> 13 	xul.dll	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message&&)
> 14 	xul.dll	mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)
> 15 	xul.dll	mozilla::ipc::MessageChannel::MessageTask::Run()
> 16 	xul.dll	nsThread::ProcessNextEvent(bool, bool*)
> 17 	xul.dll	NS_InvokeByIndex
> 18 	xul.dll	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
> 19 	xul.dll	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)

Still reported this crash even after fixing bug 1354443.
https://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/printing/nsPrintEngine.cpp#1838-1839
>     nsIPageSequenceFrame* seqFrame =
>       printData->mPrintObject->mPresShell->GetPageSequenceFrame();

Could mPresShell be nullptr? However, mPresShell is cleared only when mPrintObject is destroyed. Oddly, mPrintObject won't be destroyed until printData is destroyed. So, does this crash mean that it fails to initialize the objects but continues to process print preview?
If mPrintObject is nullptr, it should crash during calling GetDisplayTitleAndURL() above. So, only mPresShell can be nullptr here.
Hmm, mPresShell isn't set if mDontPrint or mInvisible is true. However, they are set to true only when print object is for a sub frame. So, looks like that mPrt->mPrintObject->mPresShell shouldn't be able to be nullptr.
I still don't understand how to reproduce this.

If I ignore various listener calls, root print object's mPresShell and mPresContext are always initialized while mIsDoingPrintPreview is true (if nsPrintEngine::ReflowPrintObject() fails to initialize print data and print object, it returns error. Then, DoCommonPrint() returns error. Finally, CommonPrint() sets mIsDoingPrintPreview to false. If mIsDoingPrintPreview is false, nsDocumentViewer::Destroy() never calls nsPrintEngine::SetupToPrintContent() via nsPrintEngine::FinishPrintPreview(). So, there could be some ways to destroy nsDocumentViewer during initializing print preview...
Oh, I found this stack:

https://crash-stats.mozilla.com/report/index/745bba4b-eff5-46b2-aed2-c75a00170627
>  0 	xul.dll	nsPrintEngine::SetupToPrintContent()
> 1 	xul.dll	nsPrintEngine::DocumentReadyForPrinting()
> 2 	xul.dll	nsPrintEngine::FinishPrintPreview()
> 3 	xul.dll	nsPrintEngine::AfterNetworkPrint(bool)
> 4 	xul.dll	nsPrintEngine::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult)
> 5 	xul.dll	nsDocLoader::DoFireOnStateChange(nsIWebProgress* const, nsIRequest* const, int&, nsresult)
> 6 	xul.dll	nsDocLoader::FireOnStateChange(nsIWebProgress*, nsIRequest*, int, nsresult)
> 7 	xul.dll	nsDocLoader::doStopURLLoad(nsIRequest*, nsresult)
> 8 	xul.dll	nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult)
> 9 	xul.dll	mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult)
> 10 	xul.dll	mozilla::net::nsLoadGroup::Cancel(nsresult)
> 11 	xul.dll	nsDocLoader::Stop()
> 12 	xul.dll	nsDocShell::Stop(unsigned int)
> 13 	xul.dll	nsDocShell::Destroy()
> 14 	xul.dll	nsFrameLoader::DestroyDocShell()
> 15 	xul.dll	nsFrameLoaderDestroyRunnable::Run()
> 16 	xul.dll	nsDocument::MaybeInitializeFinalizeFrameLoaders()
> 17 	xul.dll	mozilla::detail::RunnableMethodImpl<mozilla::dom::CanvasRenderingContext2D* const, void ( mozilla::dom::CanvasRenderingContext2D::*)(void), 1, 0>::Run()
> 18 	xul.dll	nsContentUtils::RemoveScriptBlocker()
> 19 	xul.dll	nsPrintEngine::DoCommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*)
> 20 	xul.dll	nsPrintEngine::CommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*)
> 21 	xul.dll	nsPrintEngine::PrintPreview(nsIPrintSettings*, mozIDOMWindowProxy*, nsIWebProgressListener*)
> 22 	xul.dll	nsDocumentViewer::PrintPreview(nsIPrintSettings*, mozIDOMWindowProxy*, nsIWebProgressListener*)
> 23 	xul.dll	NS_InvokeByIndex

nsContentUtils::RemoveScriptBlocker() triggers destroying the document. This occurs before initializing mPrt->mPrintObject. So, not having mPresShell nor mPresContext makes sense in this case.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

Oops, sorry, this is not enough for the latter case.
Attachment #8881722 - Flags: review?(dholbert) → review-
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review160022

Sorry for the delay here -- I was focused on the work week and didn't get to this as soon as I should've.

::: commit-message-3f873:1
(Diff revision 3)
> +Bug 1376693 - nsPrintEngine::SetupToPrintContent() should do nothing if mPrt->mPrintObject isn't initialized r?dholbert

Two things:
 (1) Right now this expresses an expectation ("should do nothing"), but it's not entirely clear what that means in terms of the code before/after this commit.   Could you rephrase this to make it a description of the code-change instead?

E.g. "Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized"


 (2) This ^ commit message only describes maybe 10% of this changeset.  Perhaps this should be split into two changes, with the other 90% in a "part 2", so that it can have its own descriptive commit message?  (The two pieces will certainly compile & run independently of each other and seem like they're doing things that are a bit distinct, so that feels sensible to me.)

::: layout/base/nsDocumentViewer.cpp:3980
(Diff revision 3)
> -  rv = mPrintEngine->Print(aPrintSettings, aWebProgressListener);
> +  // A call of Print() may cause releasing mPrintEngine from Destroy().
> +  // Therefore, we need to grab the instance with local variable.
> +  rv = printEngine->Print(aPrintSettings, aWebProgressListener);

Two things:
 (1) This comment sounds like it's saying we only need to declare |printEngine| down here (only for the benefit of the |Print| call), and we could leave the code above this point untouched.

Can we do that?  If not, can you move this comment closer to the declaration of the local variable (|printEngine|) that it's documenting?


 (2) "may cause releasing mPrintEngine" is grammatically awkward. This would be a bit clearer: "may cause mPrintEngine to be Release()'d in Destroy()"

::: layout/base/nsDocumentViewer.cpp:4074
(Diff revision 3)
> -  rv = mPrintEngine->PrintPreview(aPrintSettings, aChildDOMWin, aWebProgressListener);
> +  // A call of PrintPreview() may cause releasing mPrintEngine from Destroy().
> +  // Therefore, we need to grab the instance with local variable.
> +  rv = printEngine->PrintPreview(aPrintSettings, aChildDOMWin,
> +                                 aWebProgressListener);

As above:
 (1) can we declare printEngine after this comment
 (2) Consider rewording the phrase "may cause releasing mPrintEngine from Destroy"

::: layout/base/nsDocumentViewer.cpp:4588
(Diff revision 3)
> +  // XXX If Destroy() has been called during calling nsPrintEngine::Print() or
> +  //     nsPrintEngine::PrintPreview(), mPrintEngine is already nullptr here.
> +  //     So, the following clean up does nothing in such case.

"XXX" implies "something's wrong here & needs to be addressed", but it's not clear to me what the comment is saying is wrong. (or whether it's even saying anything is wrong)

I'd expect/hope the Destroy() call you're hinting at would clean up things as-needed.  But maybe the subtext of your comment is that you're not sure whether it does clean up sufficiently?  If so, maybe add another line at the end, like "(Do we need some of this for that case?)"... if that's what you're getting at.

::: layout/printing/nsPrintEngine.cpp:538
(Diff revision 3)
> +  // Destroying nsAutoScriptBlocker above may cause finishing print or
> +  // print preview.  In this case, this should do nothing anymore.
> +  if (mIsDestroying || (aIsPrintPreview && !GetIsCreatingPrintPreview())) {
> +    // Returning error must make sense because the root caller shouldn't
> +    // continue to do something with this instance.
> +    return NS_ERROR_FAILURE;
> +  }

From a native English speaker's perspective, these two comments are each a bit grammatically awkward and hard to understand.

How about we merge them and rephrase as something like the following (please correct me if I've gotten any of the details wrong):
  // The nsAutoScriptBlocker above will now have been destroyed, which may
  // cause our print/print-preview operation to finish. In this case, we
  // should immediately return an error code so that the root caller knows
  // it shouldn't continue to do anything with this instance.

::: layout/printing/nsPrintEngine.cpp:1693
(Diff revision 3)
> -  if (NS_WARN_IF(!mPrt)) {
> +  if (NS_WARN_IF(!mPrt) ||
> +      NS_WARN_IF(!mPrt->mPrintObject) ||
> +      NS_WARN_IF(!mPrt->mPrintObject->mPresShell) ||
> +      NS_WARN_IF(!mPrt->mPrintObject->mPresContext)) {
>      return NS_ERROR_FAILURE;

Could you add a comment here to hint at why we have to check each of these individually?

Also, note that we have a code-comment just below this which says:
"...by holding a strong local reference to mPrt, which in turn keeps mPrintObject alive."
Is that statement in the comment still valid?  (To keep mPrintObject alive, is it sufficient to keep mPrt alive?)  And if their lifetimes are tied like that, then why do we need to check mPrt and mPrt->mPrintObject individually up here -- why isn't mPrt sufficient to prove that mPrt->mPrintObject is alive?

::: layout/printing/nsPrintEngine.cpp:3606
(Diff revision 3)
> +  // FYI: This is important for detecting whether this is called during
> +  //      initialization.  When you change here, you need to check
> +  //      DoCommonPrint() too.
>    SetIsCreatingPrintPreview(false);

I'm not clear what this comment is saying.

* What is it that's important for detecting whether this is called during initialization -- the "false" assignment?  Or just the "IsCreatingPrintPreview" state in general?

* "When you change here, you need to check DoCommonPrint" -- could you clarify what this means? I don't immediately understand what this is getting at. I see that DoCommonPrint() has a "SetIsCreatingPrintPreview(true)" call, which mirrors this one, but it sounds like you're implying there's something more subtle there that would need updating in response to changes here...
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review161826
Attachment #8881722 - Flags: review?(dholbert) → review-
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review160022

No problem. I'm also working on other lots of bugs, I work on this during my main work.

> Two things:
>  (1) Right now this expresses an expectation ("should do nothing"), but it's not entirely clear what that means in terms of the code before/after this commit.   Could you rephrase this to make it a description of the code-change instead?
> 
> E.g. "Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized"
> 
> 
>  (2) This ^ commit message only describes maybe 10% of this changeset.  Perhaps this should be split into two changes, with the other 90% in a "part 2", so that it can have its own descriptive commit message?  (The two pieces will certainly compile & run independently of each other and seem like they're doing things that are a bit distinct, so that feels sensible to me.)

Okay, I'll separate this patch to 3 patches. One is avoiding the crash, another one is stopping initializing nsPrintEngine in DoCommonPrint() if FinishPrintPreview() is called and the other is making the callers of Print() and PrintPreview() guarantee the lifetime of nsPrintEngine instance.

> Two things:
>  (1) This comment sounds like it's saying we only need to declare |printEngine| down here (only for the benefit of the |Print| call), and we could leave the code above this point untouched.
> 
> Can we do that?  If not, can you move this comment closer to the declaration of the local variable (|printEngine|) that it's documenting?
> 
> 
>  (2) "may cause releasing mPrintEngine" is grammatically awkward. This would be a bit clearer: "may cause mPrintEngine to be Release()'d in Destroy()"

I think that we can do that. However, for safer implementation, we should always use local variables since somebody will call another risky method around here. So, I'll just move the comment.

> "XXX" implies "something's wrong here & needs to be addressed", but it's not clear to me what the comment is saying is wrong. (or whether it's even saying anything is wrong)
> 
> I'd expect/hope the Destroy() call you're hinting at would clean up things as-needed.  But maybe the subtext of your comment is that you're not sure whether it does clean up sufficiently?  If so, maybe add another line at the end, like "(Do we need some of this for that case?)"... if that's what you're getting at.

Yes, I worry about some objects which are not initialized because of mPrintEngine already destroyed. (E.g., mDocument and the window.)

I'll add the comment you suggested to the next line.

> From a native English speaker's perspective, these two comments are each a bit grammatically awkward and hard to understand.
> 
> How about we merge them and rephrase as something like the following (please correct me if I've gotten any of the details wrong):
>   // The nsAutoScriptBlocker above will now have been destroyed, which may
>   // cause our print/print-preview operation to finish. In this case, we
>   // should immediately return an error code so that the root caller knows
>   // it shouldn't continue to do anything with this instance.

Thanks!

> Could you add a comment here to hint at why we have to check each of these individually?
> 
> Also, note that we have a code-comment just below this which says:
> "...by holding a strong local reference to mPrt, which in turn keeps mPrintObject alive."
> Is that statement in the comment still valid?  (To keep mPrintObject alive, is it sufficient to keep mPrt alive?)  And if their lifetimes are tied like that, then why do we need to check mPrt and mPrt->mPrintObject individually up here -- why isn't mPrt sufficient to prove that mPrt->mPrintObject is alive?

Yes. The lifetime of mPrt->mPrintObject is guaranteed by grabbing mPrt. However, if it's not been initialized yet, there is a case, mPrt is not nullptr but mPrintObject is nullptr. This change checks whether the necessary objects are initialized or not.

> I'm not clear what this comment is saying.
> 
> * What is it that's important for detecting whether this is called during initialization -- the "false" assignment?  Or just the "IsCreatingPrintPreview" state in general?
> 
> * "When you change here, you need to check DoCommonPrint" -- could you clarify what this means? I don't immediately understand what this is getting at. I see that DoCommonPrint() has a "SetIsCreatingPrintPreview(true)" call, which mirrors this one, but it sounds like you're implying there's something more subtle there that would need updating in response to changes here...

Setting it to false is important because it's necessary when methods which initialize the instance need to check if its owner already stopped using the instance.

I'll rewrite the comment with new words. Please check it.
Comment on attachment 8886173 [details]
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized

https://reviewboard.mozilla.org/r/156970/#review162576

::: commit-message-30ea2:3
(Diff revision 1)
> +nsPrintObject::mPresShell and nsPrintObject::mPresContext are initialized by
> +nsPrintEngine::ReflowPrintObject().  However, while
> +nsPrintEngine::DoCommonPrint() is initializing mPrt and mPrintObject,
> +destroying nsAutoScriptBlocker may cause calling nsDocumentViewer::Destroy()
> +or nsPrintEngine::FinishPrintPreview() directly.  Then,
> +nsPrintEngine::SetupToPrintContent() will be called.  Therefore,
> +nsPrintEngine::SetupToPrintContent() sometimes see uninitialized mPrt and
> +mPrt->mPrintObject.

This comment starts out with the intent of explaining how we might get into SetupToPrintContent with a null mPresShell & mPresContext on mPrt->mPrintObject (I think).  And I think I'm following how that might happen -- basically we call into SetupToPrintContent while we're still inside of DoCommonPrint.

But your comment ends by saying "Therefore, nsPrintEngine::SetupToPrintContent() sometimes see uninitialized mPrt and mPrt->mPrintObject" -- and I don't see how you've made *that* logical jump.  The text above it is just explaining how we can hit SetupToPrintContent before we've exited DoCommonPrint, IIUC.  It never explains how/why mPrt or mPrt->mPrintObject would be uninitialized.

Also: importantly, the badness all traces back to the script-blocker going out of scope -- and before that happens, we've already initialized mPrt->mPrintObject, here:
https://dxr.mozilla.org/mozilla-central/rev/8486950bd91878bf077a9ac33cb3c018288fe518/layout/printing/nsPrintEngine.cpp#521-522

So I'm still unclear on why we need to bother null-checking mPrintObject here.  (I think your comment explains why mPresShell/mPresContext might be null, but not mPrintObject.)

::: layout/printing/nsPrintEngine.cpp:1684
(Diff revision 1)
>  nsPrintEngine::SetupToPrintContent()
>  {
> -  if (NS_WARN_IF(!mPrt)) {
> +  // This method may be called while DoCommonPrint() initializes the instance.
> +  // In such case, this cannot do its job as expected because some objects in
> +  // mPrt have not been initialized yet but they are necessary.

Please add at the end of the first sentence here:
 "when its script blocker goes out of scope."
Comment on attachment 8886174 [details]
Bug 1376693 - part2: Make nsPrintEngine::DoCommonPrint() stop initializing the instance when the owner stops using the instance

https://reviewboard.mozilla.org/r/156972/#review162584

::: layout/printing/nsPrintEngine.cpp:539
(Diff revision 1)
> +  // The nsAutoScriptBlocker above will now have been destroyed, which may
> +  // cause our print/print-preview operation to finish. In this case, we
> +  // should immediately return an error code so that the root caller knows
> +  // it shouldn't continue to do anything with this instance.
> +  if (mIsDestroying || (aIsPrintPreview && !GetIsCreatingPrintPreview())) {
> +    // Returning error must make sense because the root caller shouldn't
> +    // continue to do something with this instance.
> +    return NS_ERROR_FAILURE;
> +  }

This "Returning error must make sense" comment... does not make sense to me. :)

I think it's redundant now, anyway -- can we just get rid of it?  (That's what I was suggesting in my previous review feedback -- I think the new language before this "if"-check covers everything that this comment was trying to say)
Attachment #8886174 - Flags: review?(dholbert) → review+
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review162586

::: commit-message-3f873:1
(Diff revision 4)
> +Bug 1376693 - part3: Make Callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls r?dholbert

unnecessary capitalization -- s/Callers/callers/

::: commit-message-3f873:3
(Diff revision 4)
> +Bug 1376693 - part3: Make Callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls r?dholbert
> +
> +This patch makes callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() grab the nsPrintPreview instance with local variable before calling them.  That guarantee that instance of nPrintEngine won't be deleted during the calls.

(1) s/nsPrintPreview instance/nsPrintEngine instance/  ("Preview" --> "Engine")

(2) s/guarantee/guarantees/

(3) You should probably wrap this to 80 characters, so that it's easier to read in a terminal. (Not the first line, because the first line has special significance -- but the subsequent explanatory line.)  This applies to all of your patches here.

 (4) You should probably add another sentence here to make it clearer that you're just moving an *already-existing* deathgrip around, rather than adding a deathgrip where we previously lacked one.   e.g. maybe add something like this: "(We already had a RefPtr in CommonPrint that basically did this.  This patch moves it out to the callers to strengthen its guarantee.)"

::: layout/base/nsDocumentViewer.cpp:4006
(Diff revision 4)
> -  if (!mPrintEngine) {
> +  // A call of nsPrintEngine::Print() may cause mPrintEngine to be Release()'d
> +  // in Destroy().  Therefore, we need to grab the instance with local variable.

(1) s/A call of/Our call to/
(2) s/with local/with a local/

(3) Also, this comment doesn't quite finish its thought -- the "therefore" doesn't quite follow.  (You're trying to prevent premature deletion, but the comment never actually mentions deletion.) So maybe extend the last sentence to make the complete thought/connection clearer:

"...grab the instance with a local variable, so that it won't be deleted during its own method."

::: layout/base/nsDocumentViewer.cpp:4100
(Diff revision 4)
> -    rv = mPrintEngine->Initialize(this, mContainer, doc,
> +  // A call of nsPrintEngine::PrintPreview() may cause mPrintEngine to be
> +  // Release()'d in Destroy().  Therefore, we need to grab the instance with
> +  // local variable.

My above three nits (for ::Print) apply here as well.

::: layout/printing/nsPrintEngine.cpp
(Diff revision 4)
>  nsresult
>  nsPrintEngine::CommonPrint(bool                    aIsPrintPreview,
>                             nsIPrintSettings*       aPrintSettings,
>                             nsIWebProgressListener* aWebProgressListener,
>                             nsIDOMDocument* aDoc) {
> -  RefPtr<nsPrintEngine> kungfuDeathGrip = this;

I take it you're removing this kungfuDeathGrip because you're now assuming that all callers must have their own local var to keep this alive, right?

Could you drop a comment here to indicate that?  E.g. "Callers must ensure that this object outlives
Attachment #8881722 - Flags: review?(dholbert) → review+
Comment on attachment 8886173 [details]
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized

https://reviewboard.mozilla.org/r/156970/#review162576

> This comment starts out with the intent of explaining how we might get into SetupToPrintContent with a null mPresShell & mPresContext on mPrt->mPrintObject (I think).  And I think I'm following how that might happen -- basically we call into SetupToPrintContent while we're still inside of DoCommonPrint.
> 
> But your comment ends by saying "Therefore, nsPrintEngine::SetupToPrintContent() sometimes see uninitialized mPrt and mPrt->mPrintObject" -- and I don't see how you've made *that* logical jump.  The text above it is just explaining how we can hit SetupToPrintContent before we've exited DoCommonPrint, IIUC.  It never explains how/why mPrt or mPrt->mPrintObject would be uninitialized.
> 
> Also: importantly, the badness all traces back to the script-blocker going out of scope -- and before that happens, we've already initialized mPrt->mPrintObject, here:
> https://dxr.mozilla.org/mozilla-central/rev/8486950bd91878bf077a9ac33cb3c018288fe518/layout/printing/nsPrintEngine.cpp#521-522
> 
> So I'm still unclear on why we need to bother null-checking mPrintObject here.  (I think your comment explains why mPresShell/mPresContext might be null, but not mPrintObject.)

Thanks. I'll add the reason why this patch also checks mPrintObject. The reason is, just safer code. I mean, some other change shouldn't cause similar crash bug as regression. I don't think that there is a good reason to not check it even with it's impossible with *current* design.

> Please add at the end of the first sentence here:
>  "when its script blocker goes out of scope."

Sure.
Comment on attachment 8886174 [details]
Bug 1376693 - part2: Make nsPrintEngine::DoCommonPrint() stop initializing the instance when the owner stops using the instance

https://reviewboard.mozilla.org/r/156972/#review162584

> This "Returning error must make sense" comment... does not make sense to me. :)
> 
> I think it's redundant now, anyway -- can we just get rid of it?  (That's what I was suggesting in my previous review feedback -- I think the new language before this "if"-check covers everything that this comment was trying to say)

Ah, yes.
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review162586

> unnecessary capitalization -- s/Callers/callers/

Oops.

> (1) s/nsPrintPreview instance/nsPrintEngine instance/  ("Preview" --> "Engine")
> 
> (2) s/guarantee/guarantees/
> 
> (3) You should probably wrap this to 80 characters, so that it's easier to read in a terminal. (Not the first line, because the first line has special significance -- but the subsequent explanatory line.)  This applies to all of your patches here.
> 
>  (4) You should probably add another sentence here to make it clearer that you're just moving an *already-existing* deathgrip around, rather than adding a deathgrip where we previously lacked one.   e.g. maybe add something like this: "(We already had a RefPtr in CommonPrint that basically did this.  This patch moves it out to the callers to strengthen its guarantee.)"

Thank you. Suggesting the words is very helpful to me!

> (1) s/A call of/Our call to/
> (2) s/with local/with a local/
> 
> (3) Also, this comment doesn't quite finish its thought -- the "therefore" doesn't quite follow.  (You're trying to prevent premature deletion, but the comment never actually mentions deletion.) So maybe extend the last sentence to make the complete thought/connection clearer:
> 
> "...grab the instance with a local variable, so that it won't be deleted during its own method."

Same.

> I take it you're removing this kungfuDeathGrip because you're now assuming that all callers must have their own local var to keep this alive, right?
> 
> Could you drop a comment here to indicate that?  E.g. "Callers must ensure that this object outlives

Sure.

BTW, in general XPCOM rules, any callers should guarantee that the lifetime during calling any methods. However, it does not make sense to use RefPtr every time since it may cause damage to the performance if it does in hot path. So, if we had some rules to indicate the dangerous method and could notify compiler or lint, we could fix a lot of security bugs though.
Comment on attachment 8886173 [details]
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized

https://reviewboard.mozilla.org/r/156970/#review162576

> Thanks. I'll add the reason why this patch also checks mPrintObject. The reason is, just safer code. I mean, some other change shouldn't cause similar crash bug as regression. I don't think that there is a good reason to not check it even with it's impossible with *current* design.

> I don't think that there is a good reason to not check it

The reason to not check it (in opt builds) is: the presence of this check here implies that we aren't sure, and it casts suspicion on whether or not we actually hold up to the invariant.  It casts suspicion on every other place where we *depend* on the invariant holding up; it raises the question, maybe we should really null-check mPrt->mPrintObject there as well, and handle its possible-nullness gracefully?

But, it's not a huge deal here I suppose; this particular check isn't perf critical, and there's some sense to null-checking the whole chain if you're checking the start and the end of it.

At a minimum, it'd probably be worth dropping a code-comment here just to acknowledge the probable-unnecessariness of this check -- e.g.:
    // Note: it shouldn't be possible for mPrt->mPrintObject to be null; we
    // just check it for good measure, as we check its owner & members.
That makes it a bit less mysterious.  (You have something to this ^^ effect in the extended commit message, but that's less discoverable to someone who might be thrown off by this check.)
Comment on attachment 8886173 [details]
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized

https://reviewboard.mozilla.org/r/156970/#review163780

r=me with the following

::: commit-message-30ea2:8
(Diff revision 2)
> +nsPrintEngine::SetupToPrintContent() will be called.  Therefore,
> +nsPrintEngine::SetupToPrintContent() sometimes see uninitialized mPrt,
> +mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext.

s/mPrt/mPrt, or/

(Right now, this sentence is saying that *all three pointers* could be simultaneously uninitialized. But that makes no sense, because if mPrt is uninitialized, then the other two pointers don't even exist.  Really you mean to say either the first, or the latter two.)

::: layout/printing/nsPrintEngine.cpp:1686
(Diff revision 2)
> -  if (NS_WARN_IF(!mPrt)) {
> +  // This method may be called while DoCommonPrint() initializes the instance
> +  // when its script blocker goes out of scope.  In such case, this cannot do
> +  // its job as expected because some objects in mPrt have not been initialized
> +  // yet but they are necessary.
> +  if (NS_WARN_IF(!mPrt) ||
> +      NS_WARN_IF(!mPrt->mPrintObject) ||
> +      NS_WARN_IF(!mPrt->mPrintObject->mPresShell) ||
> +      NS_WARN_IF(!mPrt->mPrintObject->mPresContext)) {
>      return NS_ERROR_FAILURE;
>    }
>  

Please add a note (maybe in parens, as an afterthought to this comment) with something like my suggested text in comment 27, to document that the mPrintObject check is just for good measure.
Attachment #8886173 - Flags: review?(dholbert) → review+
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review163802

Nit on the new comment in part 3:

::: layout/printing/nsPrintEngine.cpp:390
(Diff revisions 4 - 5)
>  
>  nsresult
>  nsPrintEngine::CommonPrint(bool                    aIsPrintPreview,
>                             nsIPrintSettings*       aPrintSettings,
>                             nsIWebProgressListener* aWebProgressListener,
>                             nsIDOMDocument* aDoc) {
> +  // Callers must ensure that this object outlives.
>    nsresult rv = DoCommonPrint(aIsPrintPreview, aPrintSettings,
>                                aWebProgressListener, aDoc);

This language doesn't quite make sense (which object?  And what does the object need to outlive? And why must & how should callers keep it alive?)

How about something like this:
  // Callers must hold a strong reference to |this| to ensure that we stay
  // alive for the duration of this method, because our main owning reference
  // (on nsDocumentViewer) might be cleared during this function (if we cause
  // script to run and it cancels the print operation).


(As you said, XPCOM does generally make a blanket requirement of something like this -- but this case is special, because callers might rightfully expect that the reference on nsDocumentViewer would be sufficient to keep this thing alive.  But it's not.)

Also: please put a blank line after this comment (or maybe pull it out just above the function?) so that it's more clearly a comment for this whole function, rather than for the rv = DoCommonPrint(...) statement right below it.
Comment on attachment 8881722 [details]
Bug 1376693 - part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls

https://reviewboard.mozilla.org/r/152840/#review163802

> This language doesn't quite make sense (which object?  And what does the object need to outlive? And why must & how should callers keep it alive?)
> 
> How about something like this:
>   // Callers must hold a strong reference to |this| to ensure that we stay
>   // alive for the duration of this method, because our main owning reference
>   // (on nsDocumentViewer) might be cleared during this function (if we cause
>   // script to run and it cancels the print operation).
> 
> 
> (As you said, XPCOM does generally make a blanket requirement of something like this -- but this case is special, because callers might rightfully expect that the reference on nsDocumentViewer would be sufficient to keep this thing alive.  But it's not.)
> 
> Also: please put a blank line after this comment (or maybe pull it out just above the function?) so that it's more clearly a comment for this whole function, rather than for the rv = DoCommonPrint(...) statement right below it.

> How about something like this:

Sure, I'll replace it with your suggestion.

> (As you said, XPCOM does generally make a blanket requirement of something like this -- but this case is special, because callers might rightfully expect that the reference on nsDocumentViewer would be sufficient to keep this thing alive.  But it's not.)

Only holding refcountable objects only from class members if they might be set to nullptr by Destroy() or something. I meet a lot of security bugs which are caused by this reason... So, not "special" case, unfortunately.

> Also: please put a blank line after this comment

Sure.
Comment on attachment 8886173 [details]
Bug 1376693 - part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized

https://reviewboard.mozilla.org/r/156970/#review162576

> > I don't think that there is a good reason to not check it
> 
> The reason to not check it (in opt builds) is: the presence of this check here implies that we aren't sure, and it casts suspicion on whether or not we actually hold up to the invariant.  It casts suspicion on every other place where we *depend* on the invariant holding up; it raises the question, maybe we should really null-check mPrt->mPrintObject there as well, and handle its possible-nullness gracefully?
> 
> But, it's not a huge deal here I suppose; this particular check isn't perf critical, and there's some sense to null-checking the whole chain if you're checking the start and the end of it.
> 
> At a minimum, it'd probably be worth dropping a code-comment here just to acknowledge the probable-unnecessariness of this check -- e.g.:
>     // Note: it shouldn't be possible for mPrt->mPrintObject to be null; we
>     // just check it for good measure, as we check its owner & members.
> That makes it a bit less mysterious.  (You have something to this ^^ effect in the extended commit message, but that's less discoverable to someone who might be thrown off by this check.)

> it raises the question, maybe we should really null-check mPrt->mPrintObject there as well, and handle its possible-nullness gracefully?

Yeah, as far as possible, I think so (although, not scope of this bug). Crash during printing may cause serious dataloss for users. For example, when user buys something and the result page has something important information like serial code, but the page cannot be accessed later, user may try to print it. In such case, crashing during print preview or printing is really critical. So, if we can avoid crash without any regressions, as far as we should do it.

> That makes it a bit less mysterious.  (You have something to this ^^ effect in the extended commit message, but that's less discoverable to someone who might be thrown off by this check.)

Sure, I'll add it.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7c792959ed4c
part1: Make nsPrintEngine::SetupToPrintContent() return early if mPrt->mPrintObject isn't initialized r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f1cae5c666a6
part2: Make nsPrintEngine::DoCommonPrint() stop initializing the instance when the owner stops using the instance r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bf9fcbecce13
part3: Make callers of nsPrintEngine::Print() and nsPrintEngine::PrintPreview() guarantee that instance of nsPrintEngine won't be deleted during the calls r=dholbert
Given how fragile our printing code is, I'm thinking we should let this ride the trains. What do you think, Masayuki-san?
Flags: needinfo?(masayuki)
I think so, this crash is not so many. So, I don't think we should take it into Beta in this late stage.
Flags: needinfo?(masayuki)
Depends on: 1391645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: