Closed Bug 1391645 Opened 7 years ago Closed 7 years ago

'An error occurred while printing" message is constantly displayed while trying to print vertical frames

Categories

(Core :: Printing: Output, defect, P2)

Product:
Core
Component:
Printing: Output
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 --- verified

People

(Reporter: emilghitta, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files)

printing_vertical_frames.html
1010 bytes, text/html
Details
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false
59 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-beta+
Details 
[Affected versions]:
Firefox 57.0a1 
Firefox 56.0b4 

[Affected platforms]:
Windows 10 64bit

[Unaffected platforms]:
Ubuntu 16.04 64bit.
macOS 10.11.6.

[Steps to reproduce]:
1.Launch Firefox.
2.Print the attached test file.

[Expected result]:
The test file is sucessfully printed.

[Actual result]:
The file is not printed and the "An error occured while printing" error message is thrown.

[Regression range]:
I think that this is a regression and I will get back to it asap.

[Additional Information]
Please note that this issue is reproducible with both e10s enabled/disabled.
Some updates:
I noticed that this issue occurs only when the "Each frame separately" option is checked from the "Print Frames" options.
Also we managed to reproduce this on Ubuntu and mac.
OS: Windows 10 → All
Hardware: x86_64 → All
[Tracking Requested - why for this release]: Print error due to regression.
tracking-firefox56: --- → ?
[Steps to reproduce]:
1. Launch Firefox.
2. Print the attached test file.
3. Check "Each frame separately" option

This is a 100% reproducible issue on my local test.
Component: Printing → Printing: Output
Priority: -- → P2
Product: Toolkit → Core
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review176002

::: commit-message-9f36f:1
(Diff revision 1)
> +Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() keep handling to print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr r?dholbert
> +
> +When user prints only documents loaded in frames, instance of nsPrintEngine for
> +root document which has <frameset> element won't initialize
> +mPrt->mPrintObject->mPresShell nor mPrt->mPrintObject->mPresShell.  In this
> +case, nsPrintEngine::SetupPrintContent() needs to keep handling the printing.
> +Therefore, it shouldn't return error even if they are nullptr.

Why were we checking mPresShell/mPresContext here before?

Presumably we have some code *somewhere* that depends on them being non-null, or else you wouldn't have added this check.  (And I take it that code must not be reachable or must not matter in this case)  Can you extend the commit message to be clearer about that?

::: layout/printing/nsPrintEngine.cpp:1701
(Diff revision 1)
>    // yet but they are necessary.
>    // 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.
> +  // just check it for good measure, as we check its owner.  However, its
> +  // mPresShell and mPresContext may be nullptr, e.g., when only documents
> +  // in frames are printed separately and this instance is for the root
> +  // document having <frameset>.
>    if (NS_WARN_IF(!mPrt) ||
> -      NS_WARN_IF(!mPrt->mPrintObject) ||
> +      NS_WARN_IF(!mPrt->mPrintObject)) {

Probably don't bother mentioning mPresShell and mPresContext here -- it feels a bit excessive, because this method doesn't mention them anywhere.

Also, is it still worth checking mPrintObject here? Could we just MOZ_ASSERT that it's non-null, after the mPrt early-return?  (The old justification was largely that we were null-checking its members, so we might as well null-check it as well, even though it's guaranteed to be non-null when mPrt is non-null. But now we're not null-checking its members.)
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review176002

> Why were we checking mPresShell/mPresContext here before?
> 
> Presumably we have some code *somewhere* that depends on them being non-null, or else you wouldn't have added this check.  (And I take it that code must not be reachable or must not matter in this case)  Can you extend the commit message to be clearer about that?

Ah, I misunderstood the comment.

According to the commit message, checking mPresShell and mPresContext are important:
https://reviewboard.mozilla.org/r/156970/diff/3/download/4722900/new/

So, we need to keep checking them for normal cases.

> Probably don't bother mentioning mPresShell and mPresContext here -- it feels a bit excessive, because this method doesn't mention them anywhere.
> 
> Also, is it still worth checking mPrintObject here? Could we just MOZ_ASSERT that it's non-null, after the mPrt early-return?  (The old justification was largely that we were null-checking its members, so we might as well null-check it as well, even though it's guaranteed to be non-null when mPrt is non-null. But now we're not null-checking its members.)

So, for checking mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext, we need the check.
Track 56+ as new regression.
tracking-firefox56: ? → +
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review176418

::: commit-message-9f36f:7
(Diff revision 2)
> +handling the printing.  Therefore, it shouldn't return error even if they are
> +nullptr.  Otherwise, it needs to keep checking if they are nullptr for avoiding
> +crash.

"for avoiding crash" is kinda vague here. Could you clarify (or at least hint at) at why/where nullptrs in these variables would trigger a crash, AND why we don't have to worry about triggering that crash in the frameset case?  (It's not enough to simply say that the variables are expected to be null in that case.)

e.g. maybe replace the sentence with something like this:

"But in the non-frameset case, we have later code in function $FOO which depends on these variables being non-null, so we need to preserve this null-check-and-early-failure to avoid crashing in that case. (Notably, $FOO is skipped for the frameset case.)"

I'm hoping we can say something like that, but I don't know what $FOO is or whether/how its checks are skipped for framesets.

::: layout/printing/nsPrintEngine.cpp:1705
(Diff revision 2)
> +  // Note: mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext
> +  // may be nullptr if the document for this instance has frames with <frameset>
> +  // and it's printing only the content of the frames (i.e., user chose "The
> +  // selected frame" or "Each frame separately" in the print dialog).  So, when
> +  // mPrt->mPrintObject->mFrameType is eFrameSet, we shouldn't check them.

So looking at this comment, I'm still left with several questions:
 (1) Does the mFrameType == eFrameSet condition *mean* the user chose one of these special options? Or does it just mean that the document *has* a frameset? (but we don't know if the user selected this option or not?)


 (2) If it's problematic in the normal case for mPresShell/Context to be null (problematic enough that we have to bail out), why is it fine in the frameset case?  (i.e. presumably there's some later code that depends on these pointers being non-null, in the normal case. How/why does that later code get skipped in the frameset case?)

 (3) Suppose we trigger the original problematic scenario that caused you to add this early-return (i.e. we get called before mPrintObject has been fully initialized or whatever), AND suppose the user has chosen this special frameset behavior.  In that scenario, should we be checking something else to tell us whether mPrintObject is initialized? (since we clearly can't depend on the nullness of mPresShell/mPresContext to tell us)  Because right now, it seems like we'd be proceeding with a partially-initialized mPrintObject (and we simply can't tell that it's partially-initialized).  Is that bad?
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review176418

> "for avoiding crash" is kinda vague here. Could you clarify (or at least hint at) at why/where nullptrs in these variables would trigger a crash, AND why we don't have to worry about triggering that crash in the frameset case?  (It's not enough to simply say that the variables are expected to be null in that case.)
> 
> e.g. maybe replace the sentence with something like this:
> 
> "But in the non-frameset case, we have later code in function $FOO which depends on these variables being non-null, so we need to preserve this null-check-and-early-failure to avoid crashing in that case. (Notably, $FOO is skipped for the frameset case.)"
> 
> I'm hoping we can say something like that, but I don't know what $FOO is or whether/how its checks are skipped for framesets.

Actual crash point is here:
https://hg.mozilla.org/releases/mozilla-beta/annotate/0a6a0148a61c/layout/printing/nsPrintEngine.cpp#l1839

and the latter question of the first sentence is a good question. It's referred only when mIsCreatingPrintPreview is true. When it's in print preview, both mPresShell and mPresContext should not be nullptr because print preview always previews all frames.

So, perhaps, we should check it instead of nsPrintObject::mFrameType.

> So looking at this comment, I'm still left with several questions:
>  (1) Does the mFrameType == eFrameSet condition *mean* the user chose one of these special options? Or does it just mean that the document *has* a frameset? (but we don't know if the user selected this option or not?)
> 
> 
>  (2) If it's problematic in the normal case for mPresShell/Context to be null (problematic enough that we have to bail out), why is it fine in the frameset case?  (i.e. presumably there's some later code that depends on these pointers being non-null, in the normal case. How/why does that later code get skipped in the frameset case?)
> 
>  (3) Suppose we trigger the original problematic scenario that caused you to add this early-return (i.e. we get called before mPrintObject has been fully initialized or whatever), AND suppose the user has chosen this special frameset behavior.  In that scenario, should we be checking something else to tell us whether mPrintObject is initialized? (since we clearly can't depend on the nullness of mPresShell/mPresContext to tell us)  Because right now, it seems like we'd be proceeding with a partially-initialized mPrintObject (and we simply can't tell that it's partially-initialized).  Is that bad?

> (1) Does the mFrameType == eFrameSet condition *mean* the user chose one of these special options?
>     Or does it just mean that the document *has* a frameset? (but we don't know if the user selected
>     this option or not?)

Oh, it's the latter. So, yes, you're right. I think that we should check mIsCreatingPrintPreview instead because mPresShell referred only when it's true.

I'll check other methods called by SetupToPrintContent() since there are some other cases of mPresShell/mPresContext are referred.
So, now, I believe that when mIsCreatingPrintPreview is false, we don't need to check mPresContext nor mPresShell because we've gotten some crash reports which crashes only when mIsCreatingPrintPreview is true. So, I'd say, when mIsCreatingPrintPreview is false, nsPrintEngine handles correctly.
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review177054

::: commit-message-9f36f:1
(Diff revision 3)
> +Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() keep handling to print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false r?dholbert

"keep handling to print" isn't valid English. Maybe you mean "proceed with the print"?

::: commit-message-9f36f:3
(Diff revision 3)
> +Bug 1376693 tried to fix crashing when initializing page sequence frame during
> +creating print preview.  On the other hand, nsPrintEngine is printing documents,
> +mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresShell can be nullptr
> +if the document has <frameset> element and it's printing only content of a
> +<frame> element or all <frame> elements separately.

The "on the other hand" doesn't quite make sense here, because the first sentence doesn't full explain the thing that "on the other hand" is opposed up against.

I think you want to say:
"Bug 1376693 added a null-check to bail from print operations if mPresShell/mPresContext are null, to avoid some null-deref crashes.  However, it turns out it's possible for these variables to be null under normal conditions -- for example, ..."

::: commit-message-9f36f:6
(Diff revision 3)
> +if the document has <frameset> element and it's printing only content of a
> +<frame> element or all <frame> elements separately.
> +
> +Therefore, this patch makes nsPrintEngine::SetupToPrintContent() check
> +mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext only when
> +mIsCreatingPrintPreview is true.

This doesn't make sense. Right now it seems to be saying "this special frameset-printing mode can cause these variables to be null; therefore we check mIsCreatingPrintPreview".  And that's logic doesn't make sense -- it's not clear what the connection is (if any) between  mIsCreatingPrintPreview and frameset-printing.

Can you make it clearer why mIsCreatingPrintPreview is a reasonable thing to check here? (assuming it is reasonable)

::: layout/printing/nsPrintEngine.cpp:1648
(Diff revision 3)
> +    // If po->mDontPrint is false, mPresContext and mPresShell need to be
> +    // non-nullptr.
> +    MOZ_ASSERT(po->mPresContext && po->mPresShell);
> +

Please include a string message with assertions (the second MOZ_ASSERT parameter).  I don't particularly care what it says, as long as the message and the code-comment before the assertion (combined) explain the following things:

 (a) why we care about the assertion holding up -- e.g. "We need non-null mPresContext and mPresShell so we can $FOO".

 (b) why we're justified in making this assertion -- e.g. "$BAR should've given us a non-null mPresContext/mPresShell by now, if we're actually printing", or something like that.

(I don't know $FOO and $BAR offhand but I imagine there must be some appropriate placeholder-text for them.)

Sometimes (a)/(b) are obvious from contextual code, but they're not obvious in this case, which is why I'm asking for clarification.

::: layout/printing/nsPrintEngine.cpp:1705
(Diff revision 3)
>    // yet but they are necessary.
>    // 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.
> +  // just check it for good measure, as we check its owner.
>    if (NS_WARN_IF(!mPrt) ||
> -      NS_WARN_IF(!mPrt->mPrintObject) ||
> +      NS_WARN_IF(!mPrt->mPrintObject)) {

The remaining justification here -- "we check it for good measure, as we check its owner" -- doesn't make a whole lot of sense as a justification for checking it.  Feels a bit too hand-wavy.  There are plenty of other things we could check "for good measure" here but don't.

How about we clarify this to:
 "we check it for good measure (after we check its owner) before we start dereferencing it below."

::: layout/printing/nsPrintEngine.cpp:1719
(Diff revision 3)
> +  // mPrt->mPrintObject->mPresShell need to be non-nullptr because this cannot
> +  // initialize page sequence frame without them at end of this method since
> +  // page sequence frame has already been destroyed or not been created yet.
> +  if (mIsCreatingPrintPreview &&
> +      (NS_WARN_IF(!mPrt->mPrintObject->mPresContext) ||
> +       NS_WARN_IF(!mPrt->mPrintObject->mPresShell))) {

I'm still not clear how we get here with these pointers being null...

::: layout/printing/nsPrintEngine.cpp:1720
(Diff revision 3)
> +    return NS_ERROR_FAILURE;
> +  }
> +#ifdef DEBUG
> +  // If this is printing some documents, mPrt->mPrintObject->mPresContext and
> +  // mPrt->mPrintObject->mPresShell can be nullptr only when
> +  // mPrt->mPrintObject->mDontPrint is set to true.  E.g., if the document has
> +  // a <frameset> element and it's printing only content in a <frame> element
> +  // or all <frame> elements separately.  In other words, if this instance is
> +  // printing the document but its presContext nor presShell hasn't been
> +  // initialized, this instance can do nothing.
> +  else {
> +    MOZ_ASSERT(mPrt->mPrintObject->mDontPrint ||
> +      (mPrt->mPrintObject->mPresContext && mPrt->mPrintObject->mPresShell));
> +  }
> +#endif // #ifdef DEBUG
> +

Four suggestions/concerns here:
 (1) Drop the "else { ... }" wrapper here -- this is else-after-return, which is unnecessary.

 (2) Also drop the #ifdef DEBUG -- once the else is gone, it only contains MOZ_ASSERT, which doesn't need an #ifdef wrapper.

 (3) In the comment and the assertion-condition, please use !IsPrintable() rather than mDontPrint.  That's just an inverted getter for mDontPrint, so it's equivalent -- but I think it's preferable because that's how we check for this in ReflowPrintObject() (which is where mPresContext/mPresShell get initialized).  So it's easier to see the connection between these conditions if you use the same condition that we use in ReflowPrintObject().

 (4) I think the final sentence of the comment here is worded confusingly and might be describing some sort of condition that never happens ("if this instance is printing the document but its presContext nor presShell hasn't been initialized") -- does that ever happen? And if it does happen, we would fail this assertion (and maybe crash), right?  And when you say "if this instance is printing..." are you talking about printing as opposed to print-previewing, or as opposed to having mDontPrint set, or as opposed to something else?

::: layout/printing/nsPrintEngine.cpp:2409
(Diff revision 3)
>      nsPrintObject* po = mPrt->mPrintDocList.ElementAt(i);
>      NS_ASSERTION(po, "nsPrintObject can't be null!");
> +    // Note: if po->mDontPrint is true, po->mPresContext may be nullptr.
>      if (po->mPresContext && po->mPresContext->IsRootPaginatedDocument()) {

This comment is a bit confusing, since it starts out by talking about a condition that none of the surrounding code cares about (mDontPrint).

Maybe reword to get to the point (that mPresContext is allowed to be null here) more directly, and then explain why/how in an afterthought?

(Also, as above, probably better to refer to IsPrintable() rather than mDontPrint.)

 e.g. something like this might be clearer:
    // Note: The po->mPresContext null-check below is necessary, because
    // it's possible po->mPresContext might never have been set.
    // (e.g. if IsPrintable() returns false, ReflowPrintObject bails
    // before setting mPresContext)
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review177096

(I'm marking this r- for the moment because otherwise mozreview doesn't trigger "review request" emails when there's a new version)

(Also, BTW, you might want to use MozReview UI to explicitly discard my previous comments with "Fixed" or "Drop", after you've replied to them -- otherwise they stick around indefinitely and get intermixed with newer review feedback in the MozReview UI.  E.g. right now MozReview shows 10 open review issues, though 2 of them are from the previous version of the patch.  I think I marked 2 even-older comments as "fixed" or "drop", yesterday [those comments being from the initial review round], when I noticed that they were still there in the MozReview UI despite you having replied to them.)
Attachment #8899386 - Flags: review?(dholbert) → review-
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review177054

> This doesn't make sense. Right now it seems to be saying "this special frameset-printing mode can cause these variables to be null; therefore we check mIsCreatingPrintPreview".  And that's logic doesn't make sense -- it's not clear what the connection is (if any) between  mIsCreatingPrintPreview and frameset-printing.
> 
> Can you make it clearer why mIsCreatingPrintPreview is a reasonable thing to check here? (assuming it is reasonable)

So, only when creating print preview, we need to avoid the crash. I'll modify the commit comment.

> Please include a string message with assertions (the second MOZ_ASSERT parameter).  I don't particularly care what it says, as long as the message and the code-comment before the assertion (combined) explain the following things:
> 
>  (a) why we care about the assertion holding up -- e.g. "We need non-null mPresContext and mPresShell so we can $FOO".
> 
>  (b) why we're justified in making this assertion -- e.g. "$BAR should've given us a non-null mPresContext/mPresShell by now, if we're actually printing", or something like that.
> 
> (I don't know $FOO and $BAR offhand but I imagine there must be some appropriate placeholder-text for them.)
> 
> Sometimes (a)/(b) are obvious from contextual code, but they're not obvious in this case, which is why I'm asking for clarification.

Hmm, because if |po->mDontPrint| is false, it means that the document should be printed.  In such case, they shouldn't been non-nullptr (i.e., should've been created for the print).

> I'm still not clear how we get here with these pointers being null...

I *guess* that when destroying the script blocker in DoCommonPrint() (it occurs before finishing the initialization), the "Print Preview" is finished by user operation or script.  Then, it causes calling nsPrintEngine::FinishPrintPreview() -> nsPrintEngine::DocumentReadyForPrinting() -> nsPrintEngine::SetupToPrintContent().

I don't understand why sPrintEngine::FinishPrintPreview() needs to call nsPrintEngine::DocumentReadyForPrinting(), however, I don't want to spend my time here with risky change.

> Four suggestions/concerns here:
>  (1) Drop the "else { ... }" wrapper here -- this is else-after-return, which is unnecessary.
> 
>  (2) Also drop the #ifdef DEBUG -- once the else is gone, it only contains MOZ_ASSERT, which doesn't need an #ifdef wrapper.
> 
>  (3) In the comment and the assertion-condition, please use !IsPrintable() rather than mDontPrint.  That's just an inverted getter for mDontPrint, so it's equivalent -- but I think it's preferable because that's how we check for this in ReflowPrintObject() (which is where mPresContext/mPresShell get initialized).  So it's easier to see the connection between these conditions if you use the same condition that we use in ReflowPrintObject().
> 
>  (4) I think the final sentence of the comment here is worded confusingly and might be describing some sort of condition that never happens ("if this instance is printing the document but its presContext nor presShell hasn't been initialized") -- does that ever happen? And if it does happen, we would fail this assertion (and maybe crash), right?  And when you say "if this instance is printing..." are you talking about printing as opposed to print-previewing, or as opposed to having mDontPrint set, or as opposed to something else?

I think that it never occurs, therefore, I used MOZ_ASSERT rather than |if (NS_WARN_IF(!foo)) { return NS_ERROR_FAILURE; }|.

And I commented it about "as opposed to print-previewing".

Okay, I'll drop the last sentence and adding some comments for clearlify.
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

https://reviewboard.mozilla.org/r/170622/#review177564

r=me with the commit message adjusted to be a bit clearer:

::: commit-message-9f36f:6
(Diff revision 4)
> +conditions -- for example, nsPrintEngine is printing documents,
> +mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresShell can be nullptr

s/nsPrintEngine/when nsPrintEngine/

(I think)

::: commit-message-9f36f:11
(Diff revision 4)
> +This special frameset-printing mode can cause these variables to be null;
> +however, the crash occurred only when mIsCreatingPrintPreview is true.  So,
> +we should check the variables only when it's true.
> +
> +There are no crash reports which occurred during printing (i.e.,
> +when mIsCreatingPrintPreview is false).  So, we can assume that nsPrintEngine
> +references the variables after proper checks when it's printing.

This still feels hand-wavy in two ways:
 (1) It's still not at all clear what the relationship is between frameset-printing & this print-preview flag. (It's unclear **why** your new mIsCreatingPrintPreview check would help at all with this frameset-triggered nullptr scenario.)
 (2) The reasoning from "the crash occurred only when mIsCreatingPrintPreview is true" seems a bit tenuous / like shots in the dark.

RE (1), I think this frameset mode never happens during print preview -- is that correct? If so, that makes a bit more sense (and you should mention that here).

RE (2), I think you mean to say that the crash occurred **in code that we only visit when mIsCreatingPrintPreview is true** -- right?  That is a stronger argument for adding a mIsCreatingPrintPreview check (so that the bail-out condition matches the condition around the crashing code).

SO: if I'm understanding correctly, please replace the paragraphs that I'm quoting above with something like the following:
 =====
  Fortunately:
    * the null-deref crashes that Bug 1376693 wanted to avoid were all in code that we only visit when mIsCreatingPrintPreview is true (i.e. during print preview).
    * this special frameset-printing mode (which causes these variables to be null) _cannot be used during print preview_.

   So, we can avoid the print-preview-specific crashes without breaking frameset-printing by simply making our null-check bail-out conditional on mIsCreatingPrintPreview.
 =====

(For me at least, this ^^ feels easier to follow, more logical, & less magical.)
Attachment #8899386 - Flags: review?(dholbert) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2a5eb6d82381
Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false r=dholbert
https://hg.mozilla.org/mozilla-central/rev/2a5eb6d82381
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1376693.

[User impact if declined]:
Cannot print only frames separately nor only selected frame (default).

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Not yet, but I confirmed with local build.

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

1. Open pages which have <frameset> and some <frame>s. (e.g., attachment 8898794 [details])

2-1. Open Print dialog.
3-1. Choose "As laid out on the screen" and print it. (The result is same as you see on the screen, i.e., you should see multiple <frames> in a page)

2-2. Choose "Each frame separately" and pint it. (Each documents are printed as different pages)

2-3. Don't click anything in the document after loading the pages.
3-3. Open Print dialog.
4-3. Choose "The selected frame" and print it. (The result is same as 3-1)

2-4. Click on one of the <frame>s.
3-4. Open Print dialog.
4-4. Choose "The selected frame" and print it. (Only the clicked <frame> should be printed)


Then, you see no error and printing the document(s), it's okay. (This patch doesn't touch actual printing result.)

If you'd check Print Preview also works fine, I'd be happy.

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

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This patch makes the early return added by bug 1376693 work only when creating print preview.

[String changes made/needed]:
No.
Attachment #8899386 - Flags: approval-mozilla-beta?
Hi Emil,
Could you help verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(emil.ghitta)
Hi Gerry!

I can confirm that this issue is no longer reproducible on Firefox 57.0a1 (BuildId:20170827100428), by following the steps provided in Comment 27, using Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.

I will leave the qe-verify+ and the needinfo until this issue gets verified in beta.
status-firefox57: fixed → verified
Comment on attachment 8899386 [details]
Bug 1391645 - Make nsPrintEngine::SetupToPrintContent() proceed with the print even when mPrt->mPrintObject->mPresShell and mPrt->mPrintObject->mPresContext are nullptr but mIsCreatingPrintPreview is false

Fix verified in nightly, let's uplift for beta 7.
Attachment #8899386 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm that this issue is verified fixed on Firefox 56.0b7 (BuildId:20170828185355) using Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Flags: needinfo?(emil.ghitta)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: