Closed Bug 1086684 (e10s-lazarus) Opened 10 years ago Closed 9 years ago

[e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug

Categories

(Core :: General, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
e10s m5+ ---
firefox39 --- fixed

People

(Reporter: martin, Assigned: mrbkap)

References

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(3 files, 7 obsolete files)

User story:
Open  a new bug in bugzilla or try to attach a file to an existing bug.
Click add an attachment in the bugzilla webpage.
Click "Browse" & Select a file.
Tab will crash shortly after selecting the file and returning to the page

Expected Result:
File Upload

Reproducible: Always
BuildID: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141021030208 CSet: 29fbfc1b31aa
Blocks: e10s
Hey Martin - thanks for reporting! Can you see if you still crash if you have all of your add-ons disabled?
Flags: needinfo?(martin)
Testing , please ignore.
Flags: needinfo?(martin)
Tested with Addons disabled, crash is gone:
List of previously active Addons:
Addon-Compatibility Reporter 2.0.2
Coookies Manager+ 1.5.2
Download YouTube Videos as MP4 1.7.28
DownThemall 2.0.17
eCleander 1.4
Lazarus:form Recovery 2.3
Nightly Tester Tools 3.7
Saved Password Editor 2.7.3
Secure Password Genarator 0.5
StartupMaster 1.6
Flags: needinfo?(mconley)
Looks like you were able to attach, so that's good.

Can you disable them one at a time and see if you can narrow in on the one causing the issue?
Blocks: e10s-addons
Flags: needinfo?(mconley) → needinfo?(martin)
Did retest as requested.
I found the one which is causing the crash:
The culprit is Lazarus:Form Recovery 2.3
Flags: needinfo?(martin) → needinfo?(mconley)
Thanks!
Flags: needinfo?(mconley)
Summary: [e10s] Crash when attaching file to bugzilla bug → [e10s] Lazarus: Form Recovery causes crash when attaching file to bugzilla bug
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
@Chris,
I have retested with e10s disabled, the crash does not happen. 
So the bug is triggered by e10s being enabled.
Flags: needinfo?(cpeterson)
Lazarus 3.0 does not seem to work at all with e10s enabled.
Thanks for testing, Martin. This definitely looks like an e10s bug.
Flags: needinfo?(cpeterson)
Summary: [e10s] Lazarus: Form Recovery causes crash when attaching file to bugzilla bug → [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug
I can also trigger this crash (or one that has the same signature like this one)  when selecting the Lazarus menue entry "Recover Form" from the context menu of a text box (Such as the comment textbox in bugzilla)
retesting method from comment 12 shows that I can not repro that way on build
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141114030206 CSet: 7f0d92595432
No longer blocks: e10s, e10s-addons
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
Keywords: crash, reproducible
Summary: [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug → Message Body is uneditable on latest Daily Error in MsgComposeCommands shown in Console
Sorry messed up this bug when testing, hope I did undo the unwanted changes (completely).
Blocks: e10s, e10s-addons
Crash Signature: Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
Keywords: crash, reproducible
Summary: Message Body is uneditable on latest Daily Error in MsgComposeCommands shown in Console → [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug
Did repro this bug using the story from comment 1
new crash report for this is at bp bp-9aa69788-bd4c-440a-93ab-b2a802141115
BuildID Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141114030206 CSet: 7f0d92595432
Having the same issue here, I'd say the bug is confirmed (should this be in Add-on compatibility?)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Adding "e10s-lazarus" Bugzilla alias because we are getting multiple bug reports related to Lazarus crashes.
Alias: e10s-lazarus
The developer has been notified through AMO. However, it looks like the add-on hasn't been updated in over 3 years.
@Jorge comment 20 there is a version 3 of the addon at http://getlazarus.com/download where the current development is happening
The Firefox Addons Site still has 2.3 https://addons.mozilla.org/de/firefox/addon/lazarus-form-recovery/
I see. Well, if a compatible version isn't submitted to AMO, we'll downgrade the listing to reduce its visibility.
Hi Jorge, I have tried V3 of the addon and since for a time it did not seem to work at ALL with the current nightlies, I chose to stick with V 2.3 .
I'll retry V3 when I find the time to do so.
Any update here? I don't want to give up Lazarus, which means I'm not using e10s :(.
Assignee: nobody → wmccloskey
Just verified that the crash is still here, I guess we would need the addon developer to fix this issue...
Still crashes with the same user story on: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150123093302 CSet: a6bbabebed2f
Blake, would you be able to take this? I think we can just make GetFilePath an urgent message. It appears to be unused except for the file picker in e10s as well as in testing.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#4497
Flags: needinfo?(mrbkap)
Sure. I'll get to this next week.
Assignee: wmccloskey → mrbkap
Flags: needinfo?(mrbkap)
(In reply to Bill McCloskey (:billm) from comment #29)
> Blake, would you be able to take this? I think we can just make GetFilePath
> an urgent message. It appears to be unused except for the file picker in
> e10s as well as in testing.
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#4497

To update the bug here, I looked into this and it turned out to be more complicated than just making the function urgent. The problem is that blobs can be created both in the child and the parent, and parents aren't allowed to send urgent messages to children. Fixing that requires a major rejiggering of the blob code, so I'm going to fix this by stashing the return value at a safe time in the child and avoiding the sync IPC call in HTMLInputElement.value.
Attached patch Patch v1 (obsolete) — Splinter Review
I'll write a test for this tomorrow.
Comment on attachment 8575689 [details] [diff] [review]
Patch v1

I couldn't get the test for this to work. I'm going to try again tomorrow, but in the meantime, I don't see any harm in getting review on this now.

Basically, the idea here is to avoid calling GetMozFullPath at random times and to do so only when we know it's safe (and we're not dealing with a CPOW).
Attachment #8575689 - Flags: review?(ehsan)
Comment on attachment 8575689 [details] [diff] [review]
Patch v1

Review of attachment 8575689 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLInputElement.cpp
@@ +1691,5 @@
>        return NS_OK;
>  
>      case VALUE_MODE_FILENAME:
>        if (nsContentUtils::IsCallerChrome()) {
> +        aValue.Assign(mFirstFilePath);

Please add a comment to describe why calling GetMozFullPath here would be a mistake.
Attachment #8575689 - Flags: review?(ehsan) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
I added the requested comment.
Attached patch Add a mochitest (obsolete) — Splinter Review
It's a little wonky that input.value is empty for blob files; nevertheless, I've tested and this mochitest crashes without my patch for the same reason as Lazarus would.
Attachment #8575689 - Attachment is obsolete: true
Attachment #8576890 - Flags: review?(mconley)
Comment on attachment 8576890 [details] [diff] [review]
Add a mochitest

Review of attachment 8576890 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/tests/file_bug1086684.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=42976

Wrong bug number.

@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=42976
> +-->
> +<head>
> +  <title>Test for Bug 42976</title>

Wrong bug number.

@@ +7,5 @@
> +  <title>Test for Bug 42976</title>
> +  <meta charset="UTF-8">
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=42976">Mozilla Bug 42976</a>

Wrong bug number.

@@ +10,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=42976">Mozilla Bug 42976</a>
> +<p id="display"></p>
> +<div id="content">
> +    <input type="file" id="f" name="file">

Is the name useful at all?

@@ +12,5 @@
> +<p id="display"></p>
> +<div id="content">
> +    <input type="file" id="f" name="file">
> +</div>
> +<pre id="test">

Needed?

@@ +13,5 @@
> +<div id="content">
> +    <input type="file" id="f" name="file">
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">

What's this script tag for?

::: dom/ipc/tests/test_bug1086684.html
@@ +57,5 @@
> +      let blob = new Blob([]);
> +      let file = new File([blob], "helloworld.txt", { type: "text/plain" });
> +
> +      mm.sendAsyncMessage("testBug1086684:parentReady", { file });
> +      lastResult = yield;

Standard mochitests desperately need bug 1078657. :/ add_task would make this so much more linear and readable.

That's not on you - just a gripe.

@@ +59,5 @@
> +
> +      mm.sendAsyncMessage("testBug1086684:parentReady", { file });
> +      lastResult = yield;
> +
> +      // Isn't it weird that we don't get a path at all (even though we have a

Is it weird? Like, weird as in we should file a bug?
Attachment #8576890 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #38)
> Is it weird? Like, weird as in we should file a bug?

I talked to sicking about this and he explained that this is a side-effect of how things are mocked-up. Basically, it's surprising, but simply an odd interaction between DOM files and the (internal) blobs that back them.
Attached patch Add a mochitest, v2 (obsolete) — Splinter Review
I cleaned up the internal file.
Attachment #8576890 - Attachment is obsolete: true
Attachment #8576942 - Flags: review?(mconley)
Comment on attachment 8576942 [details] [diff] [review]
Add a mochitest, v2

Review of attachment 8576942 [details] [diff] [review]:
-----------------------------------------------------------------

1 additional nit, but land away. Thanks!

::: dom/ipc/tests/file_bug1086684.html
@@ +8,5 @@
> +  <meta charset="UTF-8">
> +</head>
> +<body>
> +<a target="_blank"
> +    href="https://bugzilla.mozilla.org/show_bug.cgi?id=1086684">Mozilla Bug 1086684</a>

Nit - alignment.
Attachment #8576942 - Flags: review?(mconley) → review+
Attached patch Fix b2g mochitests (obsolete) — Splinter Review
This patch causes b2g mochitests to crash because we actually assert that we don't use this message at all in b2g (unless we're fuzzing).  With bent out, I didn't want to bother changing that, so this patch just makes the patch not assert. I suppose this makes sense, since chrome code touching content should be really rare in b2g. For more context, see <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp?rev=f77767481b16#4498>.
Attachment #8577479 - Flags: review?(ehsan)
Comment on attachment 8577479 [details] [diff] [review]
Fix b2g mochitests

Review of attachment 8577479 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but please check with Ben what the right thing to do here is, when he gets back?  Thanks!
Attachment #8577479 - Flags: review?(ehsan) → review+
Product: Firefox → Core
Attached patch Really fix b2g (obsolete) — Splinter Review
I didn't realize that MOZ_CHILD_PERMISSIONS wasn't automatically defined everywhere. This makes attachment 8577479 [details] [diff] [review] actually work as intended.

Ben signed off on this approach in person.
Attachment #8578216 - Flags: review?(gps)
Comment on attachment 8578216 [details] [diff] [review]
Really fix b2g

Review of attachment 8578216 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/moz.build
@@ +244,5 @@
>      '/netwerk/base',
>  ]
>  
> +if CONFIG['MOZ_CHILD_PERMISSIONS']:
> +    DEFINES['MOZ_CHILD_PERMISSIONS'] = True

Alternatively, you could add an AC_DEFINE for this variable in configure.in: https://dxr.mozilla.org/mozilla-central/source/configure.in#5716

That would make this define global, rather than directory specific. I don't know the code well enough to know what is best.
Attachment #8578216 - Flags: review?(gps) → review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> Also
> https://treeherder.mozilla.org/logviewer.html#?job_id=7871959&repo=mozilla-
> inbound on multiple B2G suites

This is a bug in b2g and is a crash waiting to happen.
Depends on: 1145854
Attached patch Patch v2 (obsolete) — Splinter Review
I added an assertion when I checked that last patch in and it went orange (as well as going orange for bug 1145854, which my newly-added mochitest exposed). In b2g, a file picker with a picked file was a ticking time bomb waiting for us to blur the page and send the poisoned message to the parent.

I'll hold off on review until I see the results of my most recent try run.
Attachment #8576887 - Attachment is obsolete: true
Attachment #8577479 - Attachment is obsolete: true
Attachment #8578216 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=306766674c4b

I give up on trying to add any assertions or generally trying to enforce anything related to b2g or not here. The problem that the last try push pointed out was that there are tests that use SpecialPowers to interact with <input type=file>s on b2g, which caused the new assertions to continue failing.

Rather than try to flush out all of the tests that might do this (note: the tests don't assert because the files backing them are same-process blobs and not remote blobs, which is hard to detect from HTMLInputElement) I'm just going to back off, fix this bug for desktop and hope that we don't get too badly burned in the future.

This try run should go green.
Attached patch patch v3Splinter Review
Attachment #8576942 - Attachment is obsolete: true
Attachment #8580954 - Attachment is obsolete: true
Attachment #8582017 - Flags: review?(bent.mozilla)
Comment on attachment 8582017 [details] [diff] [review]
patch v3

Review of attachment 8582017 [details] [diff] [review]:
-----------------------------------------------------------------

</holding-nose>

::: dom/html/HTMLInputElement.cpp
@@ +1691,5 @@
>        return NS_OK;
>  
>      case VALUE_MODE_FILENAME:
>        if (nsContentUtils::IsCallerChrome()) {
> +#ifndef MOZ_CHILD_PERMISSIONS

Nit: reverse these blocks to avoid the double negative

@@ +2677,5 @@
>  
>  void
>  HTMLInputElement::FireChangeEventIfNeeded()
>  {
> +  // NB: It is important to avoid calling GetValue for <input type=file> here.

Nit: Explain why?
Attachment #8582017 - Flags: review?(bent.mozilla) → review+
Blocks: 1147188
khuey pointed out the massive thinko in my previous patches (MOZ_CHILD_PERMISSIONS was inconsistently defined in various places, and would give HTMLInputElement differing sizes and member offsets depending on what directory a given file was in when it included it). This patch should fix that by defining or not MOZ_CHILD_PERMISSIONS correctly in all directories in the tree.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a778889fd7b8
Attachment #8582748 - Flags: review?(gps)
Attachment #8582748 - Flags: review?(gps) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb0b42f9068

My mochitest fails on Android (and then causes test_cpow_cookies.html to fail after it) due to timing out with no input after "prefs are set". I suspect that we can't load over HTTP in child processes on Android, but I'm not set up to test it now.

That being said, the test does not test anything that is currently relevant for Android, so it's not terrible to not run it on that platform.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: