Closed Bug 1112465 Opened 5 years ago Closed 5 years ago

Crash when navigating from HTML page to about:config while inspector is shown (without e10s)

Categories

(Core :: DOM: Core & HTML, defect, critical)

36 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 + fixed

People

(Reporter: Hb, Assigned: pbro)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-975ac09e-fe89-4718-9db9-bb80d2141216.
=============================================================

STR
1. Open a web page
2. Open Inspector
3. Select Font section in Inspector
4. Type about:config in the Address Bar
5. Hit Enter 
Crash
With a new profile this crash can't be repeated. Former crashes with this issue were bp-83fd09f1-4a65-491f-b4b1-e87a92141217	, bp-88080806-0766-476f-a487-23a072141216 and bp-dd3c9af6-8b05-4897-b8c9-f09032141216
Status: NEW → UNCONFIRMED
Ever confirmed: false
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

I can reproduce with new profile bp-625db745-7942-490b-8d8a-51c182141217

https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=0fa23eddfe12&tochange=8e9b2357abcb

Triggered by: Bug 985597
Blocks: 985597
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Product: Firefox → Core
Version: Trunk → 36 Branch
Flags: needinfo?(pbrosset)
The crash seems to be caused by nsIDocument::RemoveAnonymousContent(mozilla::dom::AnonymousContent&, mozilla::ErrorResult&).
This method is called when the devtools highlighter (in the inspector) is removed from the document. So, typically, when navigating from a page to about:config here.
If this is indeed the case, I don't think this is related to the font inspector at all, but rather just to the fact that the inspector is opened.
I haven't been able to reproduce so far on Mac at all.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> The crash seems to be caused by
> nsIDocument::RemoveAnonymousContent(mozilla::dom::AnonymousContent&,
> mozilla::ErrorResult&).
> This method is called when the devtools highlighter (in the inspector) is
> removed from the document. So, typically, when navigating from a page to
> about:config here.
> If this is indeed the case, I don't think this is related to the font
> inspector at all, but rather just to the fact that the inspector is opened.
> I haven't been able to reproduce so far on Mac at all.

You should need to try on non-e10s window.
Flags: needinfo?(pbrosset)
Ok, I found a way to reproduce the error. It's somewhat related to e10s. about:config isn't opened in the child process, its tab is in the parent process. And it seems that when we move from an e10s tab (the page you first open) to a non e10s tab (the about:config page), this closes the toolbox, and that prevents the crash from occurring.

So, to replicate the crash:

- open about:sessionrestore (this page is also in a non-e10s tab and is HTML, so the inspector uses the new highlighter)
- open the inspector
- navigate to about:config
-> crash
Flags: needinfo?(pbrosset)
Assignee: nobody → pbrosset
Summary: Crash when opening about:config while Font Inspector is shown → Crash when navigating from HTML page to about:config while inspector is shown (without e10s)
OS: Windows 7 → All
Hardware: x86_64 → All
Happens when, without e10s, you navigate from an HTML page to a XUL page (like about:newtab), with the inspector open. So it's not only related to about:config.
That's weird because this case was specifically taken care of in bug 1020244 and bug 985597, which were the bugs that were used to implement the new highlighter.
The new highlighter isn't compatible with XUL, but that's why we specifically put in place a mechanism that would fall back to another, simpler, highlighter when navigating to XUL.
So this mechanism must be failing somehow.
I'm currently clobber building a debug build to try and find a fix.
Here is the relevant part of the backtrace:

* thread #1: tid = 0x4d4ba9, 0x0000000102c59d8c XUL`nsCOMPtr<mozilla::dom::Element>::get(this=0x00000000000000a0) const + 12 at nsCOMPtr.h:686, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xa0)
  * frame #0: 0x0000000102c59d8c XUL`nsCOMPtr<mozilla::dom::Element>::get(this=0x00000000000000a0) const + 12 at nsCOMPtr.h:686
    frame #1: 0x0000000102c58865 XUL`nsCOMPtr<mozilla::dom::Element>::operator mozilla::dom::Element*(this=0x00000000000000a0) const + 21 at nsCOMPtr.h:694
    frame #2: 0x0000000103206fac XUL`nsCanvasFrame::GetCustomContentContainer(this=0x0000000000000000) const + 28 at nsCanvasFrame.h:104
    frame #3: 0x00000001031c7511 XUL`nsIDocument::RemoveAnonymousContent(this=0x0000000118de4000, aContent=0x000000012c3e3200, aRv=0x00007fff5fbece48) + 97 at nsDocument.cpp:5245
I think this should fix it. What was happening is that by the time devtools tries to remove the inserted AnonymousContent, the document it does it on is already the destination XUL document.
And it turns out only InsertAnonymousContent was checking for |shell->GetCanvasFrame()|, not RemoveAnonymousContent.
So this patch only adds this simple test to avoid crashing.
Attachment #8537804 - Flags: review?(ehsan.akhgari)
Attachment #8537804 - Flags: review?(ehsan.akhgari) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81bca1a05670
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Patrick, can we have an uplift request for aurora? Thanks
Flags: needinfo?(pbrosset)
Comment on attachment 8537804 [details] [diff] [review]
bug1112465-check-for-canvasframe-in-removeAnonymousContent v1.patch

Approval Request Comment
[Feature/regressing bug #]: This is a regression introduced by a refactor of the devtools highlighter done in bug 985597.
[User impact if declined]: If declined, users opening the devtools inspector on an HTML page and then navigating to a XUL page will have a crash.
[Describe test coverage new/current, TBPL]: The highlighter is fairly heavily tested by devtools mochitests, but somehow, we didn't have a test for this very specific scenario, hence the regression. The patch didn't introduce a new test though. I'll file a follow-up bug to add one, but I think the patch can be uplifted without one.
[Risks and why]: Not much risk involved, the fix is a simple one line check.
[String/UUID change made/needed]: None.
Flags: needinfo?(pbrosset)
Attachment #8537804 - Flags: approval-mozilla-aurora?
Attachment #8537804 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.