1354447 - nsViewManager::mRootView may be outdated

Status: RESOLVED WORKSFORME

(Core :: Web Painting, defect)

Description

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

According to the stack trace of bug 1342552, nsViewManager::mRootView may be outdated. In most cases, it must be safe, e.g., destroying nsView clears its view manager's mRootView from its destructor, setting root view to an instance which was created immediately before.
http://searchfox.org/mozilla-central/search?q=symbol:_ZN13nsViewManager11SetRootViewEP6nsView&redirect=false

However, nsPrintEngine may reuse pointer to old nsView instance:
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#2084,2086,2102
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#2081,2089,2093

So, we should fix this with making nsView refcountable or deprecating nsViewManager::SetRootView() and creating nsViewManager::CreateNewRootViewFor() and creating nsViewManager::ClearRootView() if nsPrintEngine doesn't need to reuse old nsView instance.

I also filed bug 1354443 for life time issue of printing objects.

Comment 1

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

Oops...

Comment 2

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

Accidentally, I filed this bug as open. So, it caused sending emails to watchers. I'll prepare the files ASAP. I'm really sorry.

(I hope, there would be another form to enter only security bug or the checkbox first or something...)

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Masayuki Nakano [:masayuki] from comment #0)
> According to the stack trace of bug 1342552, nsViewManager::mRootView may be
> outdated. In most cases, it must be safe, e.g., destroying nsView clears its
> view manager's mRootView from its destructor, setting root view to an
> instance which was created immediately before.
> http://searchfox.org/mozilla-central/search?q=symbol:
> _ZN13nsViewManager11SetRootViewEP6nsView&redirect=false
> 
> However, nsPrintEngine may reuse pointer to old nsView instance:
> http://searchfox.org/mozilla-central/rev/
> 381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.
> cpp#2084,2086,2102
> http://searchfox.org/mozilla-central/rev/
> 381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.
> cpp#2081,2089,2093

In nsPringEngine we only call SetRootView on either a freshly created view, or a pointer we got from mRootView. And according to your first paragraph the existing mRootView should be valid because all other setters of mRootView must set a valid pointer and it would get cleared if the view went away. Am I missing something?

Comment 4

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

Thank you for the quick response.

I'm still not sure how the nsView pointer becomes outdated actually. However, if there were no stacktrace of bug 1342552, I must not think that there is a bug in nsView management. Is it possible to appear the stacktrace with calling a method of nsViewManager after the view manager instance is destroyed? If it's possible, this bug may be invalid and there may be a bug of managing nsViewManager's life cycle.

Comment 5

[Mats Palmgren (inactive)]

Could someone CC me on bug 1342552 please?

Comment 6

[Markus Stange [:mstange]]

done

Comment 7

[Andrew McCreight [:mccr8]]

I'll just mark this high, though maybe it isn't that bad if it is only printing related.

Comment 8

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

Fortunately, our known crash will be perfectly gone if the patch for bug 1342552. However, it only hide this issue...

Comment 9

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

s/if the patch for bug 1342552/if the patch for bug 1342552 will be landed

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Masayuki Nakano [:masayuki] from comment #8)
> Fortunately, our known crash will be perfectly gone if the patch for bug
> 1342552. However, it only hide this issue...

Unfortunately this isn't the only bug where it looks like memory corruption with views is happening.

Comment 11

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

Oh, really? I don't understand why nsView isn't refcountable...

Comment 12

[Stephanie Ouillon [:arroway] (needinfo me)]

Masayuki, is there any update regarding this bug? It hasn't been active for the past 3 weeks.

Comment 13

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

I'm working on bug 1354443 which may have caused bug 1342552. So, I think that I'm not sure if this is valid even after fixing bug 1354443.

(I tried to create the patch for this bug, but it causes a lot of oranges in tryserver. So, it's very difficult to make it refcountable...)

Comment 14

[Frederik Braun [:freddy]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> I'm working on bug 1354443 which may have caused bug 1342552. So, I think
> that I'm not sure if this is valid even after fixing bug 1354443.
> 
> (I tried to create the patch for this bug, but it causes a lot of oranges in
> tryserver. So, it's very difficult to make it refcountable...)


The mentioned bugs have been closed. Care to revisit this?

Comment 15

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

We should watch the result of the fix after it's released. If all crashes would have gone, we wouldn't need to work on this.

Comment 16

[Stephanie Ouillon [:arroway] (needinfo me)]

Masayuki, what can you tell about the result of the fix now that it has been released?

Comment 17

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

Although, bug 1354443 hasn't been released yet in non-ESR channel, the number of crash bugs caused by accessing mRootView is really reduced and I see only a couple of crash reports in Beta channel like:
https://crash-stats.mozilla.com/report/index/9d791b86-3e3c-4aa2-bbee-03ad10170706
https://crash-stats.mozilla.com/report/index/aa0f226d-2d69-4b02-9cf7-66dcc0170706
Perhaps, we don't need to make nsView nor nsViewManager refcountable for this issue. But I'm not sure how to fix the remaining crashes.

Comment 18

[Wennie]

Hi Jet:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 19

[Frederik Braun [:freddy]]

Dan, do you think we can close this?
Looking at crashes for nsViewManager::GetRootWidget, we dont see anything in newer versions of Firefox.
Maybe I'm getting the signature wrong, given most links here have turned invalid. :/

Comment 20

[Daniel Veditz [:dveditz]]

Most of the nsViewManager crashes I see are in ~nsViewManager (93%), and of those ~4% are null derefs and the rest are at a breakpoint added by bug 1261175.

~2% of the crashes are the one fixed in bug 1342552 and those went away in Firefox 54 as expected. Beyond that there are scattered signatures and I'm not sure they're the ones described in this bug.

I think we can close this.