1342872 - Label runnables in layout/printing/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

All calls to NS_DispatchTo(Main|Current)Thread under layout/printing/.

http://searchfox.org/mozilla-central/search?q=NS_DispatchTo(Main%7CCurrent)Thread&case=false&regexp=true&path=layout%2Fprinting

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Calls related to TimerCallback.

http://searchfox.org/mozilla-central/search?q=TimerCallback&case=false&regexp=false&path=layout%2Fprinting

Comment 2

[Astley Chen (inactive)]

Farmer, as you are working on platform printing work now, would you please help on labeling in printing module ? Thanks.

Comment 3

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Review commit: https://reviewboard.mozilla.org/r/121318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121318/

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review commit: https://reviewboard.mozilla.org/r/121320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121320/

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Hi Bevis, could you help take a look at this patch?

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Hi Bevis, could you help take a look at this patch?

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review123780

The assertion makes thing more clear that this is a main-thread only function.
The use of the API is correct but please help to follw the naming convention.

Thanks!

::: layout/printing/nsPrintEngine.cpp:3629
(Diff revision 1)
>  void
>  nsPrintEngine::FirePrintCompletionEvent()
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
>    nsCOMPtr<nsIRunnable> event = new nsPrintCompletionEvent(mDocViewerPrint);
> -  if (NS_FAILED(NS_DispatchToCurrentThread(event)))
> +  nsresult rv = mDocument->Dispatch("nsDocumentViewer::OnDonePrinting",

1. Please follow [the runnable naming convention](https://wiki.mozilla.org/Quantum/DOM#Runnable_names).
2. The original layout plus {} clousure looks more compact.
```cpp
  if (NS_FAILED(mDocument->Dispatch("nsDocumentViewer::OnDonePrinting",
                                    TaskCategory::Other, event.forget()))) {
    NS_WARNING("failed to dispatch print completion event");
  }
```

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review123788

Please see my comments below.

Thanks!

::: layout/printing/nsPagePrintTimer.cpp:161
(Diff revision 1)
>        donePrePrint = mPrintEngine->PrePrintPage();
>      }
>  
>      if (donePrePrint && !mWaitingForRemotePrint) {
>        StopWatchDogTimer();
> -      NS_DispatchToMainThread(this);
> +      nsCOMPtr<nsIContentViewer> cv(do_QueryInterface(mDocViewerPrint));

The original flow won't be failure when cv or cv->GetDocument() is invalid.
Please have more nullptr check and proper error handling before accessing  nsIDocument::Dispatch(). For example,
```cpp
      nsCOMPtr<nsIContentViewer> cv = do_QueryInterface(mDocViewerPrint);
      if(NS_WARN_IF(!cv)) {
        Fail();
        return NS_ERROR_UNEXPECTED;
      }
      nsCOMPtr<nsIDocument> doc = cv->GetDocument();
      if(NS_WARN_IF(!cv)) {
        Fail();
        return NS_ERROR_UNEXPECTED;
      }
      nsCOMPtr<nsIRunnable> event(this);
      doc->Dispatch("nsPagePrintTimer", mozilla::TaskCategory::Other, event.forget());
```

Comment 9

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121318/diff/1-2/

Comment 10

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/1-2/

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review125228

f=me, after the comment is addressed, thanks!

::: layout/printing/nsPagePrintTimer.cpp:167
(Diff revision 2)
> +      if(NS_WARN_IF(!cv)) {
> +        Fail();
> +        return NS_ERROR_UNEXPECTED;
> +      }
> +      nsCOMPtr<nsIDocument> doc = cv->GetDocument();
> +      if(NS_WARN_IF(!cv)) {

if(NS_WARN_IF(!doc))

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review125230

f=me, thanks!

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/2-3/

Comment 14

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/3-4/

Comment 15

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/4-5/

Comment 16

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Labeling story: https://wiki.mozilla.org/Quantum/DOM
Hi jwatt,
DOM team would like us to label runables.
I followed guideline to modify them for printing and got f+ from DOMteam.
Could you help review them?

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review125424

::: commit-message-61286:1
(Diff revision 2)
> +Bug 1342872 - part1: labeling runable dispatched by NS_DispatchTo(Main|Current)Thread in nsPrintEngine

I'd recommend the commit message summary line be written in the imperitive voice. So "Label runnable..." rather than "Labeling runnable..."

Also notice the connect spelling of "runnable". It's two "n"s.

And this commit only labels one runnable dispatched by NS_DispatchToCurrentThread.

So please make this:

"Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine"

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #17)
> Also notice the connect spelling of "runnable". It's two "n"s.

Hah-hah. Oh, the irony. I meant *correct* spelling of course, not "connect" spelling.

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121318/diff/2-3/

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/5-6/

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review125382

::: layout/printing/nsPagePrintTimer.cpp:44
(Diff revision 5)
>          delay = mDelay + ((10 - mFiringCount) * 100);
>        } else {
>          delay = mDelay;
>        }
>      }
> +    nsCOMPtr<nsIContentViewer> cv(do_QueryInterface(mDocViewerPrint));

Repeating all this QI code bloats the source and requires us to do more work at runtime. To avoid that please add an Init() function that returns true on success, false on failure, get the document there, and store it in a new nsCOMPtr<nsIDocument> member.

::: layout/printing/nsPagePrintTimer.cpp:45
(Diff revision 5)
>        } else {
>          delay = mDelay;
>        }
>      }
> +    nsCOMPtr<nsIContentViewer> cv(do_QueryInterface(mDocViewerPrint));
> +    if(NS_WARN_IF(!cv)) {

Please insert a space between "if" and "(". You have this in lots of other places too. Please search your diff and fix that everywhere.

::: layout/printing/nsPagePrintTimer.cpp:52
(Diff revision 5)
> +    }
> +    nsCOMPtr<nsIDocument> doc = cv->GetDocument();
> +    if(NS_WARN_IF(!doc)) {
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +    mTimer->SetTarget(doc->EventTargetFor(mozilla::TaskCategory::Other));

Add |using namespace mozilla;| at the top of this file and drop the 'mozilla::' from here and the other places you refer to TaskCategory. It makes the code less verbose and malginally easier to read.

Comment 22

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review125424

> I'd recommend the commit message summary line be written in the imperitive voice. So "Label runnable..." rather than "Labeling runnable..."
> 
> Also notice the connect spelling of "runnable". It's two "n"s.
> 
> And this commit only labels one runnable dispatched by NS_DispatchToCurrentThread.
> 
> So please make this:
> 
> "Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine"

Change to "Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine"

Comment 23

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/6-7/

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review125664

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review125522

::: layout/printing/nsPagePrintTimer.h:41
(Diff revision 7)
>      , mPrintObj(nullptr)
>      , mWatchDogCount(0)
>      , mDone(false)
>    {
>      mDocViewerPrint->IncrementDestroyRefCount();
> +    Init();

The point of an initialization function would be to throw away the nsPagePrintTimer and abort if it fails. However, if Init() is called from inside the constructor it doesn't help us do that (we might as well inline the body of Init in the constructor). It needs to be the code that *creates* the nsPagePrintTimer that calls Init() and checks for failure.

::: layout/printing/nsPagePrintTimer.cpp:37
(Diff revision 7)
> +  nsCOMPtr<nsIContentViewer> cv(do_QueryInterface(mDocViewerPrint));
> +  if (NS_WARN_IF(!cv)) {
> +      return;
> +  }
> +  mDocument = cv->GetDocument();
> +}

You would need to check for success here too. So:

  if (NS_WARN_IF(!doc)) {
    return false;
  }
  return true;

::: layout/printing/nsPagePrintTimer.cpp:180
(Diff revision 7)
>      }
>  
>      if (donePrePrint && !mWaitingForRemotePrint) {
>        StopWatchDogTimer();
> -      NS_DispatchToMainThread(this);
> +      nsCOMPtr<nsIContentViewer> cv(do_QueryInterface(mDocViewerPrint));
> +      if(NS_WARN_IF(!cv)) {

Use mDocument here.

Comment 26

[Jonathan Watt [:jwatt]]

I guess there's also a concern that the code that creates the nsPagePrintTimer will forget to call Init(). You can protect against that by making the constructor and Init methods private, and adding a factory method similar to the CreateOrNull factory methods we have elsewhere in the printing code.

Comment 27

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/7-8/

Comment 28

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/8-9/

Comment 29

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review126072

Just a couple of issues to fix. Thanks, farmer!

::: layout/printing/nsPagePrintTimer.h:39
(Diff revision 9)
> +
>    nsresult Start(nsPrintObject* aPO);
>  
>    NS_IMETHOD Run() override;
>  
> +  bool Init();

This should only be called by CreateOrNull, so it should be private too. Actually, come to think of it you can get rid of Init and just inline its code inside CreateOrNull.

::: layout/printing/nsPagePrintTimer.cpp:195
(Diff revision 9)
>        donePrePrint = mPrintEngine->PrePrintPage();
>      }
>  
>      if (donePrePrint && !mWaitingForRemotePrint) {
>        StopWatchDogTimer();
> -      NS_DispatchToMainThread(this);
> +      nsCOMPtr<nsIRunnable> event(this);

You don't need this nsCOMPtr in order to addref. Instead you can do:

    mDocument->Dispatch(nullptr, TaskCategory::Other, do_AddRef(this);

::: layout/printing/nsPrintEngine.cpp:3570
(Diff revision 9)
>      // this gives the user more time to press cancel
>      int32_t printPageDelay = 50;
>      mPrt->mPrintSettings->GetPrintPageDelay(&printPageDelay);
>  
>      RefPtr<nsPagePrintTimer> timer =
> -      new nsPagePrintTimer(this, mDocViewerPrint, printPageDelay);
> +      nsPagePrintTimer::CreateOrNull(this, mDocViewerPrint, printPageDelay);

As the name CreateOrNull suggests, this could return null. ;) You need to check for null here and return an error code in that case. (Returning NS_ERROR_FAILURE is fine.)

Comment 30

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/9-10/

Comment 31

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Passing mDocument to the nsPagePrintTimer constructor is an another way.
We don't need CreateOrNull().
How do you think?

Comment 32

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review126214

Well spotted! That looks good to me.

::: commit-message-a9cf1:1
(Diff revision 10)
> +Bug 1342872 - part2: label the runnable and set event targets for TimerCallBack in nsPagePrintTimer

"label" is a bit of an abused term in the Quantum DOM project IMO. How about being more explicit? Maybe use the message:

Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

::: layout/printing/nsPagePrintTimer.h:32
(Diff revision 10)
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    nsPagePrintTimer(nsPrintEngine* aPrintEngine,
>                     nsIDocumentViewerPrint* aDocViewerPrint,
> -                   uint32_t aDelay)
> -    : mPrintEngine(aPrintEngine)
> +                   uint32_t aDelay,
> +                   nsIDocument* aDocument)

I'd put aDocument as the third argument just to keep it and aDocViewerPrint together.

::: layout/printing/nsPagePrintTimer.h:43
(Diff revision 10)
>      , mFiringCount(0)
>      , mPrintObj(nullptr)
>      , mWatchDogCount(0)
>      , mDone(false)
>    {
>      mDocViewerPrint->IncrementDestroyRefCount();

MOZ_ASSERT(aDocument)

Comment 33

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/10-11/

Comment 34

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review126072

> This should only be called by CreateOrNull, so it should be private too. Actually, come to think of it you can get rid of Init and just inline its code inside CreateOrNull.

Pass mDocument to the nsPagePrintTimer constructor instead of using CreateOrNull().

> As the name CreateOrNull suggests, this could return null. ;) You need to check for null here and return an error code in that case. (Returning NS_ERROR_FAILURE is fine.)

Pass mDocument to the nsPagePrintTimer constructor instead of using CreateOrNull().

Comment 35

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review126400

Comment 36

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc1947586a6b5d29cf540b46bfb810de094faee3&selectedJob=86867939

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44eae28def02
part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine r=jwatt
https://hg.mozilla.org/integration/autoland/rev/ee20d9c4b63c
part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup r=jwatt

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44eae28def02
https://hg.mozilla.org/mozilla-central/rev/ee20d9c4b63c

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

backout per irc discussion with :fatseng because we hit a crash if printing from print preview:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8025dae7ee71a96a67ada51610837461c7b308
https://hg.mozilla.org/integration/mozilla-inbound/rev/8662776f3733567441c3ad377e60c964fe7d4e56

Crash while running nsPrintCompletionEvent runnable:

#01: mozilla::ValidatingDispatcher::ValidateAccess() const[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x22ed546]
8:15 AM #02: mozilla::dom::DocGroup::ValidateAccess() const[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x227c941]
8:15 AM #03: nsDocument::GetEventTargetParent(mozilla::EventChainPreVisitor&)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x228d743]
8:15 AM [GFX3-]: Surface size too large (exceeds extent limit)!
8:15 AM [GFX2-]: Allowing surface with invalid size (Cairo) Size(47600,67360)
8:15 AM #04: mozilla::EventTargetChainItem::GetEventTargetParent(mozilla::EventChainPreVisitor&)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x365fa45]
8:15 AM #05: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3661683]
8:15 AM #06: mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x36624b9]
8:15 AM #07: nsINode::DispatchEvent(nsIDOMEvent*, bool*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2320715]
8:15 AM #08: nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*, bool)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fe4f6b]
8:15 AM #09: nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fe4cb6]
8:15 AM #10: nsDocument::UpdateVisibilityState()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x229225c]
8:15 AM #11: nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x229260d]
8:15 AM #12: nsDocumentViewer::PageHide(bool)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4d9110c]
8:15 AM #13: nsDocShell::FirePageHideNotificationInternal(bool, bool)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x684cdf8]
8:15 AM #14: nsDocShell::FirePageHideNotification(bool)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x684cce7]
8:15 AM #15: nsDocShell::Destroy()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x683f541]
8:15 AM #16: non-virtual thunk to nsDocShell::Destroy()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6863ccc]
8:15 AM #17: nsPrintObject::~nsPrintObject()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x52357b8]
8:15 AM #18: nsPrintObject::~nsPrintObject()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5226545]
8:15 AM #19: nsPrintData::~nsPrintData()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5225d76]
8:15 AM #20: nsPrintData::~nsPrintData()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x52265a5]
8:15 AM #21: mozilla::DefaultDelete<nsPrintData>::operator()(nsPrintData*) const[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x523917b]
8:15 AM #22: mozilla::UniquePtr<nsPrintData, mozilla::DefaultDelete<nsPrintData> >::reset(nsPrintData*)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x52390ff]
8:15 AM #23: mozilla::UniquePtr<nsPrintData, mozilla::DefaultDelete<nsPrintData> >::operator=(std::nullptr_t)[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5227087]
8:15 AM #24: nsPrintEngine::DestroyPrintingData()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x52270fd]
8:15 AM #25: nsDocumentViewer::OnDonePrinting()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4d99caf]
8:15 AM #26: non-virtual thunk to nsDocumentViewer::OnDonePrinting()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4d9c509]
8:15 AM #27: nsPrintCompletionEvent::Run()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x52383b1]
8:15 AM #28: mozilla::ValidatingDispatcher::Runnable::Run()[/Volumes/firefoxos/gecko-dev-0104/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1e6581]

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout
https://hg.mozilla.org/mozilla-central/rev/5a8025dae7ee71a96a67ada51610837461c7b308
https://hg.mozilla.org/mozilla-central/rev/8662776f3733567441c3ad377e60c964fe7d4e56

Comment 41

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121318/diff/3-4/

Comment 42

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121320/diff/11-12/

Comment 43

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

While launching print preview, it creates another tab. The tab group is different from the original one and mDocument was replaced somewhere. It should get correct nsIDocument from mDocViewer instead of using mDocument.

Comment 44

[Jonathan Watt [:jwatt]]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review127934

Comment 45

[Jonathan Watt [:jwatt]]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review127938

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848408 [details]
Bug 1342872 - part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine

https://reviewboard.mozilla.org/r/121318/#review128010

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848409 [details]
Bug 1342872 - part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup

https://reviewboard.mozilla.org/r/121320/#review128014

Comment 48

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=512f906d504e6d2818df57b28c55bdfd7ddae7f0&selectedJob=87797973

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0abce7b631f
part1: Label the runnable dispatched by NS_DispatchToCurrentThread in nsPrintEngine r=bevistseng,jwatt
https://hg.mozilla.org/integration/autoland/rev/fc96236eed2d
part2: Name the nsPagePrintTimer runnable and associate its nsITimers with a DocGroup r=bevistseng,jwatt

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0abce7b631f
https://hg.mozilla.org/mozilla-central/rev/fc96236eed2d